Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use "transient" files for blind writes, take 2

  1. smgrsettransient mechanism is full of bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-15T01:18:28Z

    I got a bit suspicious of the transient-file mechanism introduced in
commit fba105b1099f4f5fa7283bb17cba6fed2baa8d0c after noticing that
CleanupTempFiles seemed to take an unreasonable amount of time in a
test case that didn't involve any temp files, cf
http://archives.postgresql.org/message-id/7110.1349392471@sss.pgh.pa.us

After further review, I have become convinced that in fact it's
completely broken and needs to be redone from scratch.  The temp-file
marking at the fd.c level can easily get out of sync with the marking
at the smgr level, and that marking isn't too consistent with reality
either, which means we have all of the following problems:

(1) It can leak kernel descriptors, as reported in
http://archives.postgresql.org/message-id/B9BEA448-978F-4A14-A088-3FD82214FFAC@pvv.ntnu.no
The triggering sequence for that appears to be:
	* Transaction 1 does a blind write, sets FD_XACT_TRANSIENT.
	* At transaction close, we close kernel FD and clear
	  FD_XACT_TRANSIENT in the VFD, but the VFD, the smgr relation,
	  and the md.c data structure are all still there.
	* Transaction 2 does another blind write on same file.  This
	  does not cause FD_XACT_TRANSIENT to get set because md.c
	  data structure already exists.
	* Now we are carrying a "leaked" kernel FD that will never get
	  closed short of a CacheInvalidateSmgr message.  Which doesn't
	  happen in a dropdb scenario.  (That might be a bug in itself.)

(2) FlushBuffer will set the smgr-level transient flag even if we have
a relcache entry for the relation.  (The fact that we're doing a blind
write to flush a dirty buffer does not prove that the rel is one we're
not interested in.)  This can result in unnecessary forced closures of
kernel FDs, and it also results in too many scans of the VFD array,
because have_pending_fd_cleanup can get set unnecessarily.

(3) If the smgr-level flag gets cleared intra-transaction (ie, we did
a blind write and later started doing normal accesses to the same
relation), this fails to propagate to the VFD level, again resulting in
undesirable FD closures.

(4) After a blind write, we will close the kernel FD at transaction end,
but we don't flush the VFD array entry.  This results in VFD array bloat
over time.  The combination of this and (2) seems to explain the
performance problem I complained of above: there are too many VFD
searches done by CleanupTempFiles, and they have to pass over too many
useless entries.


I believe that we probably ought to revert this mechanism entirely, and
build a new implementation based on these concepts:

* An SMgrRelation is transient if and only if it doesn't have an
"owning" relcache entry.  Keep a list of all such SmgrRelations, and
close them all at transaction end.  (Obviously, an SMgrRelation gets
removed from the list if it acquires an owner mid-transaction.)

* There's no such concept as FD_XACT_TRANSIENT at the fd.c level.
Rather, we close and delete the VFD entry when told to by SmgrRelation
closure.

Comments?

			regards, tom lane

  2. Re: smgrsettransient mechanism is full of bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-15T19:38:50Z

    I wrote:
> I got a bit suspicious of the transient-file mechanism introduced in
> commit fba105b1099f4f5fa7283bb17cba6fed2baa8d0c after noticing that
> ...
> I believe that we probably ought to revert this mechanism entirely, and
> build a new implementation based on these concepts:
> * An SMgrRelation is transient if and only if it doesn't have an
> "owning" relcache entry.  Keep a list of all such SmgrRelations, and
> close them all at transaction end.  (Obviously, an SMgrRelation gets
> removed from the list if it acquires an owner mid-transaction.)
> * There's no such concept as FD_XACT_TRANSIENT at the fd.c level.
> Rather, we close and delete the VFD entry when told to by SmgrRelation
> closure.

Attached is a draft patch for that, presented in two parts: the first
part just reverts commit fba105b1099f4f5fa7283bb17cba6fed2baa8d0c,
and the second part installs the new mechanism.  I'm reasonably pleased
with the way this turned out; I think it's cleaner as well as more
reliable than the previous patch.

The list-management code could possibly be replaced with slist once that
patch goes in, but since this needs to be back-patched, I didn't
consider that for the moment.

Comments?

			regards, tom lane

  3. Re: smgrsettransient mechanism is full of bugs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-16T19:39:42Z

    Tom Lane wrote:

> After further review, I have become convinced that in fact it's
> completely broken and needs to be redone from scratch.  The temp-file
> marking at the fd.c level can easily get out of sync with the marking
> at the smgr level, and that marking isn't too consistent with reality
> either, which means we have all of the following problems:

Oops.  Sorry about this.  Fortunately, as far as I can see, it only
results in excessive resource consumption, not data corruption or loss.

> I believe that we probably ought to revert this mechanism entirely, and
> build a new implementation based on these concepts:
> 
> * An SMgrRelation is transient if and only if it doesn't have an
> "owning" relcache entry.  Keep a list of all such SmgrRelations, and
> close them all at transaction end.  (Obviously, an SMgrRelation gets
> removed from the list if it acquires an owner mid-transaction.)
> 
> * There's no such concept as FD_XACT_TRANSIENT at the fd.c level.
> Rather, we close and delete the VFD entry when told to by SmgrRelation
> closure.

Makes sense.  It does seem simpler than the original approach.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services