Fails to work on live images due to fsync() on pg_commit_ts before doing any write there

Raphael Hertzog wrote:

PostgreSQL 10 no longer works on a (Kali) live system where the
root filesystem is an overlayfs with an underlying squashfs
filesystem (where postgresql and its initial file structure
is present) and a writable tmpfs overlay.

Please create a machine that works this way and get it added to the
buildfarm, so that this sort of thing doesn't surprise us in the future
months after the fact.
https://wiki.postgresql.org/wiki/Buildfarm

Thanks

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Nov 7, 2017 at 10:54 PM, Raphael Hertzog <hertzog@debian.org> wrote:

This has been explained by the overlayfs upstream developer
(to which I reported this bug initially, thinking it was an
overlayfs regression):
https://marc.info/?l=linux-unionfs&amp;m=151005246512873&amp;w=2
https://marc.info/?l=linux-unionfs&amp;m=151005699414227&amp;w=2

My request is thus that PostgreSQL should fsync that directory only after
it has made changes to the directory or its content. PostgreSQL 9.6 was
working fine in the same setup and I would like PostgreSQL 10 to do the
same. :)

I'm ccing Teodor Sigaev <teodor@sigaev.ru> because I believe that
the problematic fsync() has been added by him in this commit:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1b02be21f271db6bd3cd43abb23fa596fcb6bac3

Hm. I am wondering if we should change fsync_fname_ext() so as EINVAL
is considered as a no-op. EIO and EINTR should really be caught with a
proper error, but I am not sure about this one. Thoughts?
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Alvaro, Raphael,

* Alvaro Herrera (alvherre@alvh.no-ip.org) wrote:

Raphael Hertzog wrote:

PostgreSQL 10 no longer works on a (Kali) live system where the
root filesystem is an overlayfs with an underlying squashfs
filesystem (where postgresql and its initial file structure
is present) and a writable tmpfs overlay.

Please create a machine that works this way and get it added to the
buildfarm, so that this sort of thing doesn't surprise us in the future
months after the fact.

While I agree with this, I'm not entirely convinced that this isn't an
issue with the implementation of the underlying filesystem after all. I
haven't had a chance to go read those other bug reports, but my fsync()
manpage pretty clearly seems to say that fsync should only be returning
EINVAL if it's called on a special file (FIFO, pipe, et al). There's
certainly no indication that it's ok for the same file to sometimes
support fsync() and other times *not* support fsync(). That's pretty
bizarre.

Why wouldn't it make sense for the filesystem to realize it's a no-op if
there's been no changes?

Thanks!

Stephen

On Tue, 07 Nov 2017, Stephen Frost wrote:

While I agree with this, I'm not entirely convinced that this isn't an
issue with the implementation of the underlying filesystem after all. I
haven't had a chance to go read those other bug reports, but my fsync()
manpage pretty clearly seems to say that fsync should only be returning
EINVAL if it's called on a special file (FIFO, pipe, et al). There's
certainly no indication that it's ok for the same file to sometimes
support fsync() and other times *not* support fsync(). That's pretty
bizarre.

The manpage shows "EROFS" on the same line as "EINVAL", and "EROFS" stands
for "Read-Only FileSystem". So it seems to be normal for read-only
filesystems to return errors (arguably squashfs returns EINVAL and
it would have been better to use EROFS).

And overlayfs, by its nature, is made to forward file system calls
to different underlying filesystems (that thus have different
characteristics).

That said I agree that overlayfs should be smarter and it should hide
EROFS/EINVAL on fsync() when it knows that the call is delegated to a
read-only filesystem.

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Raphael,

* Raphael Hertzog (hertzog@debian.org) wrote:

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Why?

Thanks!

Stephen

Hello Alvaro,

On Tue, 07 Nov 2017, Alvaro Herrera wrote:

Please create a machine that works this way and get it added to the
buildfarm, so that this sort of thing doesn't surprise us in the future
months after the fact.
https://wiki.postgresql.org/wiki/Buildfarm

The issue is not really related to the postgresql build. This is more the
realm of continuous integration testing (think jenkins) under a specific set
of conditions.

While I could possibly write a test script that would run on a Linux
chroot, I don't have the resources to provide a machine for your
buildfarm.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Stephen Frost <sfrost@snowman.net> writes:

* Raphael Hertzog (hertzog@debian.org) wrote:

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Why?

My thought about this is just to ignore EINVAL when fsync'ing a directory,
as we already do with EBADF.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Raphael Hertzog (hertzog@debian.org) wrote:

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Why?

My thought about this is just to ignore EINVAL when fsync'ing a directory,
as we already do with EBADF.

Yeah, I suppose we could, just not sure that an EINVAL should really be
getting returned here, imv.

Thanks!

Stephen

On 2017-11-07 15:57:03 +0100, Raphael Hertzog wrote:

Hello Alvaro,

On Tue, 07 Nov 2017, Alvaro Herrera wrote:

Please create a machine that works this way and get it added to the
buildfarm, so that this sort of thing doesn't surprise us in the future
months after the fact.
https://wiki.postgresql.org/wiki/Buildfarm

The issue is not really related to the postgresql build. This is more the
realm of continuous integration testing (think jenkins) under a specific set
of conditions.

That's pretty much what the buildfarm does.

While I could possibly write a test script that would run on a Linux
chroot, I don't have the resources to provide a machine for your
buildfarm.

This particular behaviour that you want PG to implement (no fsyncs
before something has been dirtied since the last start of PG) is weird
enough to implement and keep getting right that I personally think we a)
probably shouldn't implement it, b) there's little point in even trying
if we don't test it.

Greetings,

Andres Freund

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, 07 Nov 2017, Stephen Frost wrote:

Raphael,

* Raphael Hertzog (hertzog@debian.org) wrote:

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Why?

Because it's useless. Why call fsync() when you know that it doesn't do
anything?

And because I have just shown that doing this can have unexpected
side-effects in some specific conditions.

But this is really a matter of aesthetics and trade-offs. I don't know the
code. It might be difficult to track the fact that changes have been made.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

My thought about this is just to ignore EINVAL when fsync'ing a directory,
as we already do with EBADF.

Yeah, I suppose we could, just not sure that an EINVAL should really be
getting returned here, imv.

Yeah, it does sound more like a filesystem bug than anything else.
(I completely reject the notion that it's an application error to
fsync a directory when you haven't modified it.)

In view of the fact that we just wrapped 10.1, it'd be 3 months before
any change from our side would reach the wild. I think a key question
here is whether the Kali developers are likely to fix it from their side
in less time than that.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 2017-11-07 16:06:02 +0100, Raphael Hertzog wrote:

On Tue, 07 Nov 2017, Stephen Frost wrote:

Raphael,

* Raphael Hertzog (hertzog@debian.org) wrote:

Still I believe that this issue should be fixed in both sides. It's not
smart from PostgreSQL to call fsync() when it has not made any change.

Why?

Because it's useless. Why call fsync() when you know that it doesn't do
anything?

It's not generally useless, no. You don't know if the relevant directory
hasn't been modified since postgres was started last / crashed
last. That matters for the durability and atomicity of renames
etc. There's simply no way to track that in postgres, given it's
arbitrary external effects.

Greetings,

Andres Freund

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, 07 Nov 2017, Tom Lane wrote:

Yeah, it does sound more like a filesystem bug than anything else.
(I completely reject the notion that it's an application error to
fsync a directory when you haven't modified it.)

In view of the fact that we just wrapped 10.1, it'd be 3 months before
any change from our side would reach the wild. I think a key question
here is whether the Kali developers are likely to fix it from their side
in less time than that.

Fix what?

Following your logic, if anything needs to be fixed, it's the Linux kernel
that has to be fixed, and in particular its overlayfs filesystem. The time
to get a fix there is also relatively long. If they work on a fix quickly,
it might get merged for Linux 3.15 which we will not have in Kali before
2-3 months too.

But I can always cherry-pick a patch, either in PostgreSQL or in Linux.
BTW I'm happy to test a PostgreSQL patch for instance.

For now, I have added an ugly work-around that creates a file in the
pg_commit_ts directory (and immediately drops it) just before starting
PostgreSQL:
http://git.kali.org/gitweb/?p=packages/kali-defaults.git;a=commitdiff;h=f4287fd774f8d0686ba919f57355215a6b9633e3

Note that it's perfectly fine for me if you decide to resolve this issue
by ignoring EINVAL and/or EROFS on fsync on directories.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 2017-11-07 16:23:21 +0100, Raphael Hertzog wrote:

On Tue, 07 Nov 2017, Tom Lane wrote:

Yeah, it does sound more like a filesystem bug than anything else.
(I completely reject the notion that it's an application error to
fsync a directory when you haven't modified it.)

In view of the fact that we just wrapped 10.1, it'd be 3 months before
any change from our side would reach the wild. I think a key question
here is whether the Kali developers are likely to fix it from their side
in less time than that.

Fix what?

Following your logic, if anything needs to be fixed, it's the Linux kernel
that has to be fixed, and in particular its overlayfs filesystem.

Right. The kernel knows the file/directory ain't dirty. We don't.

The time
to get a fix there is also relatively long. If they work on a fix quickly,
it might get merged for Linux 3.15 which we will not have in Kali before
2-3 months too.

So the same timeframe as a new PG release?

For now, I have added an ugly work-around that creates a file in the
pg_commit_ts directory (and immediately drops it) just before starting
PostgreSQL:
http://git.kali.org/gitweb/?p=packages/kali-defaults.git;a=commitdiff;h=f4287fd774f8d0686ba919f57355215a6b9633e3

Hm. The comment:
+ # Ensure the directory is copied over in the tmpfs
suggests that the overlay isn't persistent? If that's indeed the case,
you could just disable fsyncs alltogether.

Greetings,

Andres Freund

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, 07 Nov 2017, Andres Freund wrote:

The time
to get a fix there is also relatively long. If they work on a fix quickly,
it might get merged for Linux 3.15 which we will not have in Kali before
2-3 months too.

So the same timeframe as a new PG release?

Yes, looks like so.

For now, I have added an ugly work-around that creates a file in the
pg_commit_ts directory (and immediately drops it) just before starting
PostgreSQL:
http://git.kali.org/gitweb/?p=packages/kali-defaults.git;a=commitdiff;h=f4287fd774f8d0686ba919f57355215a6b9633e3

Hm. The comment:
+ # Ensure the directory is copied over in the tmpfs
suggests that the overlay isn't persistent? If that's indeed the case,
you could just disable fsyncs alltogether.

Well, it's the case by default with no persistence. But some users enable
persistence and then it's no longer a tmpfs which is used.

The problem also exists when you enable persistence but start with an
empty upper layer so disabling fsyncs is not really an option.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Wed, Nov 8, 2017 at 12:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

My thought about this is just to ignore EINVAL when fsync'ing a directory,
as we already do with EBADF.

Yeah, I suppose we could, just not sure that an EINVAL should really be
getting returned here, imv.

Yeah, it does sound more like a filesystem bug than anything else.
(I completely reject the notion that it's an application error to
fsync a directory when you haven't modified it.)

In view of the fact that we just wrapped 10.1, it'd be 3 months before
any change from our side would reach the wild. I think a key question
here is whether the Kali developers are likely to fix it from their side
in less time than that.

FWIW, after sleeping on the matter and thinking about it. I agree that
we should do nothing in Postgres code. Even for a code path calling
fsync_fname_ext() and expecting a directory, I think that we really
should report EINVAL if the operation is performed on an incorrect
entry. That will be useful for any future code, but that's actually
useful for any application developer. Note that when we discussed
about 1b02be2, we thought as well about not doing the fsync when there
have been no modifications, particularly for pg_twophase, however the
flush is necessary particularly to keep durability of transactions
prepared and committed during the same checkpoint cycle. And this
keeps the code dead simple.

I tend to think that 1b02be2 ought to be back-patched as well, as not
protecting durability guarantees is no good.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Wed, Nov 8, 2017 at 1:23 AM, Raphael Hertzog <hertzog@debian.org> wrote:

Well, it's the case by default with no persistence. But some users enable
persistence and then it's no longer a tmpfs which is used.

The problem also exists when you enable persistence but start with an
empty upper layer so disabling fsyncs is not really an option.

Raphaël, don't you have problems with pg_twophase as well? Wouldn't
the new fsync call in CheckPointTwoPhase() fail equally if no 2PC
transactions have happened in a checkpoint cycle? That would not be an
issue with pg_commit_ts and pg_xact normally, but we are talking about
checkpoints skipped or not depending on WAL activity, tracking which
is more properly done in v10.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Hi Michael,

On Wed, 08 Nov 2017, Michael Paquier wrote:

Raphaël, don't you have problems with pg_twophase as well? Wouldn't
the new fsync call in CheckPointTwoPhase() fail equally if no 2PC
transactions have happened in a checkpoint cycle? That would not be an
issue with pg_commit_ts and pg_xact normally, but we are talking about
checkpoints skipped or not depending on WAL activity, tracking which
is more properly done in v10.

I don't know. It's quite possible, I have not used PostgreSQL 10
extensively in Kali... I rely on Kali users for this.

I just checked and the usual database initialization code does not trigger
any write to pg_twophase so if there's a new fsync() on that directory
without any prior write, it's quite possible that this could fail later
on during PostgreSQL's use.

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Wed, Nov 8, 2017 at 4:42 PM, Raphael Hertzog <hertzog@debian.org> wrote:

On Wed, 08 Nov 2017, Michael Paquier wrote:

Raphaël, don't you have problems with pg_twophase as well? Wouldn't
the new fsync call in CheckPointTwoPhase() fail equally if no 2PC
transactions have happened in a checkpoint cycle? That would not be an
issue with pg_commit_ts and pg_xact normally, but we are talking about
checkpoints skipped or not depending on WAL activity, tracking which
is more properly done in v10.

I don't know. It's quite possible, I have not used PostgreSQL 10
extensively in Kali... I rely on Kali users for this.

I just checked and the usual database initialization code does not trigger
any write to pg_twophase so if there's a new fsync() on that directory
without any prior write, it's quite possible that this could fail later
on during PostgreSQL's use.

You can check that by using a manual CHECKPOINT query for example.
Based on what I see on this thread, my guess is that a failure would
show up.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs