Обсуждение: pgsql: Fix a violation of WAL coding rules in the recent patch to

Log Message:
-----------
Fix a violation of WAL coding rules in the recent patch to include an
"all tuples visible" flag in heap page headers.  The flag update *must*
be applied before calling XLogInsert, but heap_update and the tuple
moving routines in VACUUM FULL were ignoring this rule.  A crash and
replay could therefore leave the flag incorrectly set, causing rows
to appear visible in seqscans when they should not be.  This might explain
recent reports of data corruption from Jeff Ross and others.

In passing, do a bit of editorialization on comments in visibilitymap.c.

Modified Files:
--------------
    pgsql/src/backend/access/heap:
        heapam.c (r1.277 -> r1.278)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/heapam.c?r1=1.277&r2=1.278)
        visibilitymap.c (r1.5 -> r1.6)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/visibilitymap.c?r1=1.5&r2=1.6)
    pgsql/src/backend/commands:
        vacuum.c (r1.389 -> r1.390)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuum.c?r1=1.389&r2=1.390)
        vacuumlazy.c (r1.121 -> r1.122)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuumlazy.c?r1=1.121&r2=1.122)
    pgsql/src/include/access:
        heapam.h (r1.143 -> r1.144)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/heapam.h?r1=1.143&r2=1.144)
        visibilitymap.h (r1.4 -> r1.5)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/visibilitymap.h?r1=1.4&r2=1.5)

Tom Lane wrote:
> Log Message:
> -----------
> Fix a violation of WAL coding rules in the recent patch to include an
> "all tuples visible" flag in heap page headers.  The flag update *must*
> be applied before calling XLogInsert, but heap_update and the tuple
> moving routines in VACUUM FULL were ignoring this rule.  A crash and
> replay could therefore leave the flag incorrectly set, causing rows
> to appear visible in seqscans when they should not be.  This might explain
> recent reports of data corruption from Jeff Ross and others.

Ouch! Yeah, the old code would write full-page images with the flag
incorrectly set.

Thanks!

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Tom Lane wrote:
> --- 74,80 ----
>    * That might actually be OK for the index scans, though. The newly inserted
>    * tuple wouldn't have an index pointer yet, so all tuples reachable from an
>    * index would still be visible to all other backends, and deletions wouldn't
> !  * be visible to other backends yet.  (But HOT breaks that argument, no?)

The same argument holds for HOT. The new tuple has no index pointer, so
it's not directly reachable from the index. Moreover, the value in the
index is correct for both tuple versions, because a HOT update doesn't
change index keys.

In fact, if we only maintained the visibility map to enable index-only
scans, a HOT update wouldn't need to clear the bit in the visibility map
at all.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com