pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Michael Paquier-2
Allow concurrent-safe open() and fopen() in frontend code for Windows

PostgreSQL uses a custom wrapper for open() and fopen() which is
concurrent-safe, allowing multiple processes to open and work on the
same file.  This has a couple of advantages:
- pg_test_fsync does not handle O_DSYNC correctly otherwise, leading to
false claims that disks are unsafe.
- TAP tests can run into race conditions when a postmaster and pg_ctl
open postmaster.pid, fixing some random failures in the buildfam.

pg_upgrade is one frontend tool using workarounds to bypass file locking
issues with the log files it generates, however the interactions with
pg_ctl are proving to be tedious to get rid of, so this is left for
later.

Author: Laurenz Albe
Reviewed-by: Michael Paquier, Kuntal Ghosh
Discussion: https://postgr.es/m/1527846213.2475.31.camel@...
Discussion: https://postgr.es/m/16922.1520722108@...

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/0ba06e0bfb8cfd24ff17aca92aa72245ddd6c4d7

Modified Files
--------------
src/bin/initdb/initdb.c                           | 8 ++++++++
src/bin/pg_basebackup/pg_receivewal.c             | 2 +-
src/bin/pg_verify_checksums/pg_verify_checksums.c | 2 +-
src/common/file_utils.c                           | 4 ++--
src/include/port.h                                | 3 ---
5 files changed, 12 insertions(+), 7 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> Allow concurrent-safe open() and fopen() in frontend code for Windows

I'm surprised you didn't back-patch this --- isn't it a bug fix?

A compromise might be to push it to v11 but not further.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Michael Paquier-2
On Fri, Sep 14, 2018 at 11:22:55AM -0400, Tom Lane wrote:
> I'm surprised you didn't back-patch this --- isn't it a bug fix?
>
> A compromise might be to push it to v11 but not further.

Yes, that's clearly a bug fix from pg_ctl point of view with TAP tests.
However, I have rather cold feet about back-patching for two reasons as
this introduces a behavior change:
- The concurrency part disappears.
- Caller of open() needs to enforce text mode to strip correctly CRLF
characters.

I would be fine to get that into v11 though, as that is not released
yet.  You will have to wait for a couple of days though, there is a long
week-end here away from laptops and electronic devices ;)
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Michael Paquier-2
On Sat, Sep 15, 2018 at 07:49:39AM +0900, Michael Paquier wrote:
> I would be fine to get that into v11 though, as that is not released
> yet.  You will have to wait for a couple of days though, there is a long
> week-end here away from laptops and electronic devices ;)

OK, REL_11_STABLE has been patched as well, after doing a couple of
extra tests on Windows.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> OK, REL_11_STABLE has been patched as well, after doing a couple of
> extra tests on Windows.

BTW, I'm a bit concerned by the fact that bowerbird has failed its
last couple of HEAD runs at the pgbench step.  The first such
failure was here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2018-09-15%2014%3A19%3A58

Looking at the set of commits between the prior run and that one,
it's hard to see anything that could have triggered the test failures
other than this patch --- but I also don't see how this patch would've
blown up pgbench without breaking earlier tests.  Ideas?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Michael Paquier-2
On Mon, Sep 17, 2018 at 09:41:37AM -0400, Tom Lane wrote:

> BTW, I'm a bit concerned by the fact that bowerbird has failed its
> last couple of HEAD runs at the pgbench step.  The first such
> failure was here:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2018-09-15%2014%3A19%3A58
>
> Looking at the set of commits between the prior run and that one,
> it's hard to see anything that could have triggered the test failures
> other than this patch --- but I also don't see how this patch would've
> blown up pgbench without breaking earlier tests.  Ideas?
Thanks, I have been looking at the build farm but I missed this one.
dory, which uses VS 2015 is not complaining because it does not run
bincheck.  At quick glance, it seems to be caused by process_file() in
pgbench.c which would need to open files in text mode, and the input
file parsing fails at the first '\' character found.  I'll test that
stuff on tomorrow morning manually.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Mon, Sep 17, 2018 at 09:41:37AM -0400, Tom Lane wrote:
>> Looking at the set of commits between the prior run and that one,
>> it's hard to see anything that could have triggered the test failures
>> other than this patch --- but I also don't see how this patch would've
>> blown up pgbench without breaking earlier tests.  Ideas?

> Thanks, I have been looking at the build farm but I missed this one.
> dory, which uses VS 2015 is not complaining because it does not run
> bincheck.  At quick glance, it seems to be caused by process_file() in
> pgbench.c which would need to open files in text mode, and the input
> file parsing fails at the first '\' character found.

Oh, you're thinking pgbench isn't robust against finding \r's visible
in its input?  Could be.

> I'll test that stuff on tomorrow morning manually.

We've got a bit of a timing problem because we want to wrap 11beta4/rc1
(still TBD) in a few hours.  I'll take a look and see if I can push a
quick fix before that.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Andrew Dunstan-8


On 09/17/2018 10:48 AM, Tom Lane wrote:

> Michael Paquier <[hidden email]> writes:
>> On Mon, Sep 17, 2018 at 09:41:37AM -0400, Tom Lane wrote:
>>> Looking at the set of commits between the prior run and that one,
>>> it's hard to see anything that could have triggered the test failures
>>> other than this patch --- but I also don't see how this patch would've
>>> blown up pgbench without breaking earlier tests.  Ideas?
>> Thanks, I have been looking at the build farm but I missed this one.
>> dory, which uses VS 2015 is not complaining because it does not run
>> bincheck.  At quick glance, it seems to be caused by process_file() in
>> pgbench.c which would need to open files in text mode, and the input
>> file parsing fails at the first '\' character found.
> Oh, you're thinking pgbench isn't robust against finding \r's visible
> in its input?  Could be.
>
>> I'll test that stuff on tomorrow morning manually.
> We've got a bit of a timing problem because we want to wrap 11beta4/rc1
> (still TBD) in a few hours.  I'll take a look and see if I can push a
> quick fix before that.


When you do I'll start a bowerbird run to check it.

cheers

andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 09/17/2018 10:48 AM, Tom Lane wrote:
>> We've got a bit of a timing problem because we want to wrap 11beta4/rc1
>> (still TBD) in a few hours.  I'll take a look and see if I can push a
>> quick fix before that.

> When you do I'll start a bowerbird run to check it.

Pushed, please test.

I think there's a general issue here of exactly how we want pgwin32_fopen
to behave, but the immediate problem is best fixed by making pgbench deal
with Windows newlines more thoroughly.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Andrew Dunstan-8


On 09/17/2018 12:13 PM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> writes:
>> On 09/17/2018 10:48 AM, Tom Lane wrote:
>>> We've got a bit of a timing problem because we want to wrap 11beta4/rc1
>>> (still TBD) in a few hours.  I'll take a look and see if I can push a
>>> quick fix before that.
>> When you do I'll start a bowerbird run to check it.
> Pushed, please test.
>
> I think there's a general issue here of exactly how we want pgwin32_fopen
> to behave, but the immediate problem is best fixed by making pgbench deal
> with Windows newlines more thoroughly.
>
>


Tests are still running, but it's past the pgbench stage on HEAD, so I
think we're good.

cheers

andrew


--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 09/17/2018 12:13 PM, Tom Lane wrote:
>> Pushed, please test.

> Tests are still running, but it's past the pgbench stage on HEAD, so I
> think we're good.

I was more concerned about whether any of the post-pgbench steps would
show a failure; but it looks like HEAD's green, so probably v11 is too.

                        regards, tom lane