public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1] Fine tune documentation for tableam
20+ messages / 4 participants
[nested] [flat]
* [PATCH v1] Fine tune documentation for tableam
@ 2019-04-04 05:48 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Justin Pryzby @ 2019-04-04 05:48 UTC (permalink / raw)
Added at commit b73c3a11963c8bb783993cfffabb09f558f86e37
---
doc/src/sgml/catalogs.sgml | 2 +-
doc/src/sgml/config.sgml | 4 ++--
doc/src/sgml/ref/select_into.sgml | 6 +++---
doc/src/sgml/storage.sgml | 17 ++++++++-------
doc/src/sgml/tableam.sgml | 44 ++++++++++++++++++++-------------------
5 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 58c8c96..40ddec4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -587,7 +587,7 @@
The catalog <structname>pg_am</structname> stores information about
relation access methods. There is one row for each access method supported
by the system.
- Currently, only table and indexes have access methods. The requirements for table
+ Currently, only tables and indexes have access methods. The requirements for table
and index access methods are discussed in detail in <xref linkend="tableam"/> and
<xref linkend="indexam"/> respectively.
</para>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4a9a1e8..90b478d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7306,8 +7306,8 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
This parameter specifies the default table access method to use when
creating tables or materialized views if the <command>CREATE</command>
command does not explicitly specify an access method, or when
- <command>SELECT ... INTO</command> is used, which does not allow to
- specify a table access method. The default is <literal>heap</literal>.
+ <command>SELECT ... INTO</command> is used, which does not allow
+ specification of a table access method. The default is <literal>heap</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/select_into.sgml b/doc/src/sgml/ref/select_into.sgml
index 17bed24..1443d79 100644
--- a/doc/src/sgml/ref/select_into.sgml
+++ b/doc/src/sgml/ref/select_into.sgml
@@ -106,11 +106,11 @@ SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replac
</para>
<para>
- In contrast to <command>CREATE TABLE AS</command> <command>SELECT
- INTO</command> does not allow to specify properties like a table's access
+ In contrast to <command>CREATE TABLE AS</command>, <command>SELECT
+ INTO</command> does not allow specification of properties like a table's access
method with <xref linkend="sql-createtable-method" /> or the table's
tablespace with <xref linkend="sql-createtable-tablespace" />. Use <xref
- linkend="sql-createtableas"/> if necessary. Therefore the default table
+ linkend="sql-createtableas"/> if necessary. Therefore, the default table
access method is chosen for the new table. See <xref
linkend="guc-default-table-access-method"/> for more information.
</para>
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 62333e3..5dfca1b 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -189,11 +189,11 @@ there.
</para>
<para>
- Note that the following sections describe the way the builtin
+ Note that the following sections describe the behavior of the builtin
<literal>heap</literal> <link linkend="tableam">table access method</link>,
- and the builtin <link linkend="indexam">index access methods</link> work. Due
- to the extensible nature of <productname>PostgreSQL</productname> other types
- of access method might work similar or not.
+ and the builtin <link linkend="indexam">index access methods</link>. Due
+ to the extensible nature of <productname>PostgreSQL</productname>, other
+ access methods might work differently.
</para>
<para>
@@ -703,11 +703,12 @@ erased (they will be recreated automatically as needed).
This section provides an overview of the page format used within
<productname>PostgreSQL</productname> tables and indexes.<footnote>
<para>
- Actually, neither table nor index access methods need not use this page
- format. All the existing index methods do use this basic format, but the
+ Actually, use of this page format is not required for either table or index
+ access methods.
+ The <literal>heap</literal> table access method always uses this format.
+ All the existing index methods also use the basic format, but the
data kept on index metapages usually doesn't follow the item layout
- rules. The <literal>heap</literal> table access method also always uses
- this format.
+ rules.
</para>
</footnote>
Sequences and <acronym>TOAST</acronym> tables are formatted just like a regular table.
diff --git a/doc/src/sgml/tableam.sgml b/doc/src/sgml/tableam.sgml
index 8d9bfd8..0a89935 100644
--- a/doc/src/sgml/tableam.sgml
+++ b/doc/src/sgml/tableam.sgml
@@ -48,54 +48,56 @@
callbacks and their behavior is defined in the
<structname>TableAmRoutine</structname> structure (with comments inside the
struct defining the requirements for callbacks). Most callbacks have
- wrapper functions, which are documented for the point of view of a user,
- rather than an implementor, of the table access method. For details,
+ wrapper functions, which are documented from the point of view of a user
+ (rather than an implementor) of the table access method. For details,
please refer to the <ulink url="https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/access/tableam.h;hb=HEAD...;
<filename>src/include/access/tableam.h</filename></ulink> file.
</para>
<para>
- To implement a access method, an implementor will typically need to
- implement a AM specific type of tuple table slot (see
+ To implement an access method, an implementor will typically need to
+ implement an AM-specific type of tuple table slot (see
<ulink url="https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/executor/tuptable.h;hb=HEAD...;
- <filename>src/include/executor/tuptable.h</filename></ulink>) which allows
+ <filename>src/include/executor/tuptable.h</filename></ulink>), which allows
code outside the access method to hold references to tuples of the AM, and
to access the columns of the tuple.
</para>
<para>
- Currently the the way an AM actually stores data is fairly
- unconstrained. It is e.g. possible to use postgres' shared buffer cache,
- but not required. In case shared buffers are used, it likely makes to
- postgres' standard page layout described in <xref
+ Currently, the way an AM actually stores data is fairly
+ unconstrained. For example, it's possible but not required to use postgres'
+ shared buffer cache. In case it is used, it likely makes
+XXX something missing here ?
+ to postgres' standard page layout described in <xref
linkend="storage-page-layout"/>.
</para>
<para>
One fairly large constraint of the table access method API is that,
currently, if the AM wants to support modifications and/or indexes, it is
- necessary that each tuple has a tuple identifier (<acronym>TID</acronym>)
+ necessary for each tuple to have a tuple identifier (<acronym>TID</acronym>)
consisting of a block number and an item number (see also <xref
linkend="storage-page-layout"/>). It is not strictly necessary that the
- sub-parts of <acronym>TIDs</acronym> have the same meaning they e.g. have
+ sub-parts of <acronym>TIDs</acronym> have the same meaning as used
for <literal>heap</literal>, but if bitmap scan support is desired (it is
optional), the block number needs to provide locality.
</para>
<para>
- For crash safety an AM can use postgres' <link
- linkend="wal"><acronym>WAL</acronym></link>, or a custom approach can be
- implemented. If <acronym>WAL</acronym> is chosen, either <link
- linkend="generic-wal">Generic WAL Records</link> can be used — which
- implies higher WAL volume but is easy, or a new type of
- <acronym>WAL</acronym> records can be implemented — but that
- currently requires modifications of core code (namely modifying
+ For crash safety, an AM can use postgres' <link
+ linkend="wal"><acronym>WAL</acronym></link>, or a custom implementation.
+ If <acronym>WAL</acronym> is chosen, either <link
+ linkend="generic-wal">Generic WAL Records</link> can be used,
+ or a new type of <acronym>WAL</acronym> records can be implemented.
+ Generic WAL Records are easy, but imply higher WAL volume.
+ Implementation of a new type of WAL record
+ currently requires modifications to core code (specifically,
<filename>src/include/access/rmgrlist.h</filename>).
</para>
<para>
To implement transactional support in a manner that allows different table
- access methods be accessed within a single transaction, it likely is
+ access methods be accessed within a single transaction, it's likely
necessary to closely integrate with the machinery in
<filename>src/backend/access/transam/xlog.c</filename>.
</para>
@@ -103,8 +105,8 @@
<para>
Any developer of a new <literal>table access method</literal> can refer to
the existing <literal>heap</literal> implementation present in
- <filename>src/backend/heap/heapam_handler.c</filename> for more details of
- how it is implemented.
+ <filename>src/backend/heap/heapam_handler.c</filename> for details of
+ its implementation.
</para>
</chapter>
--
2.1.4
--ibTvN161/egqYuK8
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v1-0001-Fine-tune-documentation-for-tableam.patch"
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-18 22:11 Andres Freund <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2022-02-18 22:11 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-02-18 13:09:45 -0800, Peter Geoghegan wrote:
> 0001 is tricky in the sense that there are a lot of fine details, and
> if you get any one of them wrong the result might be a subtle bug. For
> example, the heap_tuple_needs_freeze() code path is only used when we
> cannot get a cleanup lock, which is rare -- and some of the branches
> within the function are relatively rare themselves. The obvious
> concern is: What if some detail of how we track the new relfrozenxid
> value (and new relminmxid value) in this seldom-hit codepath is just
> wrong, in whatever way we didn't think of?
I think it'd be good to add a few isolationtest cases for the
can't-get-cleanup-lock paths. I think it shouldn't be hard using cursors. The
slightly harder part is verifying that VACUUM did something reasonable, but
that still should be doable?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-19 01:00 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-19 01:00 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Feb 18, 2022 at 2:11 PM Andres Freund <[email protected]> wrote:
> I think it'd be good to add a few isolationtest cases for the
> can't-get-cleanup-lock paths. I think it shouldn't be hard using cursors. The
> slightly harder part is verifying that VACUUM did something reasonable, but
> that still should be doable?
We could even just extend existing, related tests, from vacuum-reltuples.spec.
Another testing strategy occurs to me: we could stress-test the
implementation by simulating an environment where the no-cleanup-lock
path is hit an unusually large number of times, possibly a fixed
percentage of the time (like 1%, 5%), say by making vacuumlazy.c's
ConditionalLockBufferForCleanup() call return false randomly. Now that
we have lazy_scan_noprune for the no-cleanup-lock path (which is as
similar to the regular lazy_scan_prune path as possible), I wouldn't
expect this ConditionalLockBufferForCleanup() testing gizmo to be too
disruptive.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-19 23:08 Peter Geoghegan <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-19 23:08 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Feb 18, 2022 at 5:00 PM Peter Geoghegan <[email protected]> wrote:
> Another testing strategy occurs to me: we could stress-test the
> implementation by simulating an environment where the no-cleanup-lock
> path is hit an unusually large number of times, possibly a fixed
> percentage of the time (like 1%, 5%), say by making vacuumlazy.c's
> ConditionalLockBufferForCleanup() call return false randomly. Now that
> we have lazy_scan_noprune for the no-cleanup-lock path (which is as
> similar to the regular lazy_scan_prune path as possible), I wouldn't
> expect this ConditionalLockBufferForCleanup() testing gizmo to be too
> disruptive.
I tried this out, using the attached patch. It was quite interesting,
even when run against HEAD. I think that I might have found a bug on
HEAD, though I'm not really sure.
If you modify the patch to simulate conditions under which
ConditionalLockBufferForCleanup() fails about 2% of the time, you get
much better coverage of lazy_scan_noprune/heap_tuple_needs_freeze,
without it being so aggressive as to make "make check-world" fail --
which is exactly what I expected. If you are much more aggressive
about it, and make it 50% instead (which you can get just by using the
patch as written), then some tests will fail, mostly for reasons that
aren't surprising or interesting (e.g. plan changes). This is also
what I'd have guessed would happen.
However, it gets more interesting. One thing that I did not expect to
happen at all also happened (with the current 50% rate of simulated
ConditionalLockBufferForCleanup() failure from the patch): if I run
"make check" from the pg_surgery directory, then the Postgres backend
gets stuck in an infinite loop inside lazy_scan_prune, which has been
a symptom of several tricky bugs in the past year (not every time, but
usually). Specifically, the VACUUM statement launched by the SQL
command "vacuum freeze htab2;" from the file
contrib/pg_surgery/sql/heap_surgery.sql, at line 54 leads to this
misbehavior.
This is a temp table, which is a choice made by the tests specifically
because they need to "use a temp table so that vacuum behavior doesn't
depend on global xmin". This is convenient way of avoiding spurious
regression tests failures (e.g. from autoanalyze), and relies on the
GlobalVisTempRels behavior established by Andres' 2020 bugfix commit
94bc27b5.
It's quite possible that this is nothing more than a bug in my
adversarial gizmo patch -- since I don't think that
ConditionalLockBufferForCleanup() can ever fail with a temp buffer
(though even that's not completely clear right now). Even if the
behavior that I saw does not indicate a bug on HEAD, it still seems
informative. At the very least, it wouldn't hurt to Assert() that the
target table isn't a temp table inside lazy_scan_noprune, documenting
our assumptions around temp tables and
ConditionalLockBufferForCleanup().
I haven't actually tried to debug the issue just yet, so take all this
with a grain of salt.
--
Peter Geoghegan
From 3f01281af3ba81b35777cb7d717f76e001fd3e10 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Sat, 19 Feb 2022 14:07:35 -0800
Subject: [PATCH] Add adversarial ConditionalLockBufferForCleanup() gizmo to
vacuumlazy.c.
---
src/backend/access/heap/vacuumlazy.c | 36 +++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 242511a23..31c6b360e 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -50,6 +50,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "common/pg_prng.h"
#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
@@ -748,6 +749,39 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
}
}
+/*
+ * Adversarial gizmo, simulates excessive failure to get cleanup locks
+ */
+static inline bool
+lazy_conditionallockbufferforcleanup(Buffer buffer)
+{
+ /*
+ * Artificially fail to get a cleanup lock 50% of the time.
+ *
+ * XXX: What about temp tables? We simulate not getting a cleanup lock
+ * there, but is that choice actually reasonable?
+ */
+ if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 2))
+ return false;
+
+#if 0
+ /*
+ * 50% is very very aggressive, while 2% - 5% is still basically
+ * adversarial but in many ways less annoying.
+ *
+ * This version (which injects a failure to get a cleanup lock 2% of the
+ * time) seems to pass the regression tests, even with my parallel make
+ * check-world recipe. Expected query plans don't seem to shift on
+ * account of unexpected index bloat (nor are there any problems of a
+ * similar nature) with this variant of the gizmo.
+ */
+ if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 50))
+ return false;
+#endif
+
+ return ConditionalLockBufferForCleanup(buffer);
+}
+
/*
* lazy_scan_heap() -- workhorse function for VACUUM
*
@@ -1093,7 +1127,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
* a cleanup lock right away, we may be able to settle for reduced
* processing using lazy_scan_noprune.
*/
- if (!ConditionalLockBufferForCleanup(buf))
+ if (!lazy_conditionallockbufferforcleanup(buf))
{
bool hastup,
recordfreespace;
--
2.30.2
Attachments:
[text/plain] 0001-Add-adversarial-ConditionalLockBufferForCleanup-gizm.txt (2.3K, ../../CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q@mail.gmail.com/2-0001-Add-adversarial-ConditionalLockBufferForCleanup-gizm.txt)
download | inline diff:
From 3f01281af3ba81b35777cb7d717f76e001fd3e10 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Sat, 19 Feb 2022 14:07:35 -0800
Subject: [PATCH] Add adversarial ConditionalLockBufferForCleanup() gizmo to
vacuumlazy.c.
---
src/backend/access/heap/vacuumlazy.c | 36 +++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 242511a23..31c6b360e 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -50,6 +50,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "common/pg_prng.h"
#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
@@ -748,6 +749,39 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
}
}
+/*
+ * Adversarial gizmo, simulates excessive failure to get cleanup locks
+ */
+static inline bool
+lazy_conditionallockbufferforcleanup(Buffer buffer)
+{
+ /*
+ * Artificially fail to get a cleanup lock 50% of the time.
+ *
+ * XXX: What about temp tables? We simulate not getting a cleanup lock
+ * there, but is that choice actually reasonable?
+ */
+ if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 2))
+ return false;
+
+#if 0
+ /*
+ * 50% is very very aggressive, while 2% - 5% is still basically
+ * adversarial but in many ways less annoying.
+ *
+ * This version (which injects a failure to get a cleanup lock 2% of the
+ * time) seems to pass the regression tests, even with my parallel make
+ * check-world recipe. Expected query plans don't seem to shift on
+ * account of unexpected index bloat (nor are there any problems of a
+ * similar nature) with this variant of the gizmo.
+ */
+ if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 50))
+ return false;
+#endif
+
+ return ConditionalLockBufferForCleanup(buffer);
+}
+
/*
* lazy_scan_heap() -- workhorse function for VACUUM
*
@@ -1093,7 +1127,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
* a cleanup lock right away, we may be able to settle for reduced
* processing using lazy_scan_noprune.
*/
- if (!ConditionalLockBufferForCleanup(buf))
+ if (!lazy_conditionallockbufferforcleanup(buf))
{
bool hastup,
recordfreespace;
--
2.30.2
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 00:12 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Andres Freund @ 2022-02-20 00:12 UTC (permalink / raw)
To: [email protected]; Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
(On phone, so crappy formatting and no source access)
On February 19, 2022 3:08:41 PM PST, Peter Geoghegan <[email protected]> wrote:
>On Fri, Feb 18, 2022 at 5:00 PM Peter Geoghegan <[email protected]> wrote:
>> Another testing strategy occurs to me: we could stress-test the
>> implementation by simulating an environment where the no-cleanup-lock
>> path is hit an unusually large number of times, possibly a fixed
>> percentage of the time (like 1%, 5%), say by making vacuumlazy.c's
>> ConditionalLockBufferForCleanup() call return false randomly. Now that
>> we have lazy_scan_noprune for the no-cleanup-lock path (which is as
>> similar to the regular lazy_scan_prune path as possible), I wouldn't
>> expect this ConditionalLockBufferForCleanup() testing gizmo to be too
>> disruptive.
>
>I tried this out, using the attached patch. It was quite interesting,
>even when run against HEAD. I think that I might have found a bug on
>HEAD, though I'm not really sure.
>
>If you modify the patch to simulate conditions under which
>ConditionalLockBufferForCleanup() fails about 2% of the time, you get
>much better coverage of lazy_scan_noprune/heap_tuple_needs_freeze,
>without it being so aggressive as to make "make check-world" fail --
>which is exactly what I expected. If you are much more aggressive
>about it, and make it 50% instead (which you can get just by using the
>patch as written), then some tests will fail, mostly for reasons that
>aren't surprising or interesting (e.g. plan changes). This is also
>what I'd have guessed would happen.
>
>However, it gets more interesting. One thing that I did not expect to
>happen at all also happened (with the current 50% rate of simulated
>ConditionalLockBufferForCleanup() failure from the patch): if I run
>"make check" from the pg_surgery directory, then the Postgres backend
>gets stuck in an infinite loop inside lazy_scan_prune, which has been
>a symptom of several tricky bugs in the past year (not every time, but
>usually). Specifically, the VACUUM statement launched by the SQL
>command "vacuum freeze htab2;" from the file
>contrib/pg_surgery/sql/heap_surgery.sql, at line 54 leads to this
>misbehavior.
>This is a temp table, which is a choice made by the tests specifically
>because they need to "use a temp table so that vacuum behavior doesn't
>depend on global xmin". This is convenient way of avoiding spurious
>regression tests failures (e.g. from autoanalyze), and relies on the
>GlobalVisTempRels behavior established by Andres' 2020 bugfix commit
>94bc27b5.
We don't have a blocking path for cleanup locks of temporary buffers IIRC (normally not reachable). So I wouldn't be surprised if a cleanup lock failing would cause some odd behavior.
>It's quite possible that this is nothing more than a bug in my
>adversarial gizmo patch -- since I don't think that
>ConditionalLockBufferForCleanup() can ever fail with a temp buffer
>(though even that's not completely clear right now). Even if the
>behavior that I saw does not indicate a bug on HEAD, it still seems
>informative. At the very least, it wouldn't hurt to Assert() that the
>target table isn't a temp table inside lazy_scan_noprune, documenting
>our assumptions around temp tables and
>ConditionalLockBufferForCleanup().
Definitely worth looking into more.
This reminds me of a recent thing I noticed in the aio patch. Spgist can end up busy looping when buffers are locked, instead of blocking. Not actually related, of course.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 00:22 Peter Geoghegan <[email protected]>
parent: Peter Geoghegan <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 00:22 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 3:08 PM Peter Geoghegan <[email protected]> wrote:
> It's quite possible that this is nothing more than a bug in my
> adversarial gizmo patch -- since I don't think that
> ConditionalLockBufferForCleanup() can ever fail with a temp buffer
> (though even that's not completely clear right now). Even if the
> behavior that I saw does not indicate a bug on HEAD, it still seems
> informative.
This very much looks like a bug in pg_surgery itself now -- attached
is a draft fix.
The temp table thing was a red herring. I found I could get exactly
the same kind of failure when htab2 was a permanent table (which was
how it originally appeared, before commit 0811f766fd made it into a
temp table due to test flappiness issues). The relevant "vacuum freeze
htab2" happens at a point after the test has already deliberately
corrupted one of its tuples using heap_force_kill(). It's not that we
aren't careful enough about the corruption at some point in
vacuumlazy.c, which was my second theory. But I quickly discarded that
idea, and came up with a third theory: the relevant heap_surgery.c
path does the relevant ItemIdSetDead() to kill items, without also
defragmenting the page to remove the tuples with storage, which is
wrong.
This meant that we depended on pruning happening (in this case during
VACUUM) and defragmenting the page in passing. But there is no reason
to not defragment the page within pg_surgery (at least no obvious
reason), since we have a cleanup lock anyway.
Theoretically you could blame this on lazy_scan_noprune instead, since
it thinks it can collect LP_DEAD items while assuming that they have
no storage, but that doesn't make much sense to me. There has never
been any way of setting a heap item to LP_DEAD without also
defragmenting the page. Since that's exactly what it means to prune a
heap page. (Actually, the same used to be true about heap vacuuming,
which worked more like heap pruning before Postgres 14, but that
doesn't seem important.)
--
Peter Geoghegan
From 81f01ca623b115647ee78a1b09bbb4458fb35dab Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Sat, 19 Feb 2022 16:13:48 -0800
Subject: [PATCH 2/2] Fix for pg_surgery's heap_force_kill() function.
---
contrib/pg_surgery/heap_surgery.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index 3e641aa64..a3a193ba5 100644
--- a/contrib/pg_surgery/heap_surgery.c
+++ b/contrib/pg_surgery/heap_surgery.c
@@ -311,7 +311,8 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
*/
if (did_modify_page)
{
- /* Mark buffer dirty before we write WAL. */
+ /* Defragment and mark buffer dirty before we write WAL. */
+ PageRepairFragmentation(page);
MarkBufferDirty(buf);
/* XLOG stuff */
--
2.30.2
Attachments:
[text/plain] 0002-Fix-for-pg_surgery-s-heap_force_kill-function.txt (853B, ../../CAH2-WzkPb42odWen-HZRYw1zUe=fPAp1_JbSuVvPXQTO2iBMPw@mail.gmail.com/2-0002-Fix-for-pg_surgery-s-heap_force_kill-function.txt)
download | inline diff:
From 81f01ca623b115647ee78a1b09bbb4458fb35dab Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Sat, 19 Feb 2022 16:13:48 -0800
Subject: [PATCH 2/2] Fix for pg_surgery's heap_force_kill() function.
---
contrib/pg_surgery/heap_surgery.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index 3e641aa64..a3a193ba5 100644
--- a/contrib/pg_surgery/heap_surgery.c
+++ b/contrib/pg_surgery/heap_surgery.c
@@ -311,7 +311,8 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
*/
if (did_modify_page)
{
- /* Mark buffer dirty before we write WAL. */
+ /* Defragment and mark buffer dirty before we write WAL. */
+ PageRepairFragmentation(page);
MarkBufferDirty(buf);
/* XLOG stuff */
--
2.30.2
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 01:22 Peter Geoghegan <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 01:22 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 4:22 PM Peter Geoghegan <[email protected]> wrote:
> This very much looks like a bug in pg_surgery itself now -- attached
> is a draft fix.
Wait, that's not it either. I jumped the gun -- this isn't sufficient
(though the patch I posted might not be a bad idea anyway).
Looks like pg_surgery isn't processing HOT chains as whole units,
which it really should (at least in the context of killing items via
the heap_force_kill() function). Killing a root item in a HOT chain is
just hazardous -- disconnected/orphaned heap-only tuples are liable to
cause chaos, and should be avoided everywhere (including during
pruning, and within pg_surgery).
It's likely that the hardening I already planned on adding to pruning
[1] (as follow-up work to recent bugfix commit 18b87b201f) will
prevent lazy_scan_prune from getting stuck like this, whatever the
cause happens to be. The actual page image I see lazy_scan_prune choke
on (i.e. exhibit the same infinite loop unpleasantness we've seen
before on) is not in a consistent state at all (its tuples consist of
tuples from a single HOT chain, and the HOT chain is totally
inconsistent on account of having an LP_DEAD line pointer root item).
pg_surgery could in principle do the right thing here by always
treating HOT chains as whole units.
Leaving behind disconnected/orphaned heap-only tuples is pretty much
pointless anyway, since they'll never be accessible by index scans.
Even after a REINDEX, since there is no root item from the heap page
to go in the index. (A dump and restore might work better, though.)
[1] https://postgr.es/m/CAH2-WzmNk6V6tqzuuabxoxM8HJRaWU6h12toaS-bqYcLiht16A@mail.gmail.com
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 01:54 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Andres Freund @ 2022-02-20 01:54 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-02-19 17:22:33 -0800, Peter Geoghegan wrote:
> Looks like pg_surgery isn't processing HOT chains as whole units,
> which it really should (at least in the context of killing items via
> the heap_force_kill() function). Killing a root item in a HOT chain is
> just hazardous -- disconnected/orphaned heap-only tuples are liable to
> cause chaos, and should be avoided everywhere (including during
> pruning, and within pg_surgery).
How does that cause the endless loop?
It doesn't do so on HEAD + 0001-Add-adversarial-ConditionalLockBuff[...] for
me. So something needs have changed with your patch?
> It's likely that the hardening I already planned on adding to pruning
> [1] (as follow-up work to recent bugfix commit 18b87b201f) will
> prevent lazy_scan_prune from getting stuck like this, whatever the
> cause happens to be.
Yea, we should pick that up again. Not just for robustness or
performance. Also because it's just a lot easier to understand.
> Leaving behind disconnected/orphaned heap-only tuples is pretty much
> pointless anyway, since they'll never be accessible by index scans.
> Even after a REINDEX, since there is no root item from the heap page
> to go in the index. (A dump and restore might work better, though.)
Given that heap_surgery's raison d'etre is correcting corruption etc, I think
it makes sense for it to do as minimal work as possible. Iterating through a
HOT chain would be a problem if you e.g. tried to repair a page with HOT
corruption.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 02:16 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 2 replies; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 02:16 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 5:54 PM Andres Freund <[email protected]> wrote:
> How does that cause the endless loop?
Attached is the page image itself, dumped via gdb (and gzip'd). This
was on recent HEAD (commit 8f388f6f, actually), plus
0001-Add-adversarial-ConditionalLockBuff[...]. No other changes. No
defragmenting in pg_surgery, nothing like that.
> It doesn't do so on HEAD + 0001-Add-adversarial-ConditionalLockBuff[...] for
> me. So something needs have changed with your patch?
It doesn't always happen -- only about half the time on my machine.
Maybe it's timing sensitive?
We hit the "goto retry" on offnum 2, which is the first tuple with
storage (you can see "the ghost" of the tuple from the LP_DEAD item at
offnum 1, since the page isn't defragmented in pg_surgery). I think
that this happens because the heap-only tuple at offnum 2 is fully
DEAD to lazy_scan_prune, but hasn't been recognized as such by
heap_page_prune. There is no way that they'll ever "agree" on the
tuple being DEAD right now, because pruning still doesn't assume that
an orphaned heap-only tuple is fully DEAD.
We can either do that, or we can throw an error concerning corruption
when heap_page_prune notices orphaned tuples. Neither seems
particularly appealing. But it definitely makes no sense to allow
lazy_scan_prune to spin in a futile attempt to reach agreement with
heap_page_prune about a DEAD tuple really being DEAD.
> Given that heap_surgery's raison d'etre is correcting corruption etc, I think
> it makes sense for it to do as minimal work as possible. Iterating through a
> HOT chain would be a problem if you e.g. tried to repair a page with HOT
> corruption.
I guess that's also true. There is at least a legitimate argument to
be made for not leaving behind any orphaned heap-only tuples. The
interface is a TID, and so the user may already believe that they're
killing the heap-only, not just the root item (since ctid suggests
that the TID of a heap-only tuple is the TID of the root item, which
is kind of misleading).
Anyway, we can decide on what to do in heap_surgery later, once the
main issue is under control. My point was mostly just that orphaned
heap-only tuples are definitely not okay, in general. They are the
least worst option when corruption has already happened, maybe -- but
maybe not.
--
Peter Geoghegan
Attachments:
[application/x-gzip] corrupt-hot-chain.page.gz (140B, ../../CAH2-WznaT53OO1VCrCUHHUWxZEsXVWxP11TuNHvkZqr-frRuQQ@mail.gmail.com/2-corrupt-hot-chain.page.gz)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:01 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
1 sibling, 2 replies; 20+ messages in thread
From: Andres Freund @ 2022-02-20 03:01 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-02-19 18:16:54 -0800, Peter Geoghegan wrote:
> On Sat, Feb 19, 2022 at 5:54 PM Andres Freund <[email protected]> wrote:
> > How does that cause the endless loop?
>
> Attached is the page image itself, dumped via gdb (and gzip'd). This
> was on recent HEAD (commit 8f388f6f, actually), plus
> 0001-Add-adversarial-ConditionalLockBuff[...]. No other changes. No
> defragmenting in pg_surgery, nothing like that.
> > It doesn't do so on HEAD + 0001-Add-adversarial-ConditionalLockBuff[...] for
> > me. So something needs have changed with your patch?
>
> It doesn't always happen -- only about half the time on my machine.
> Maybe it's timing sensitive?
Ah, I'd only run the tests three times or so, without it happening. Trying a
few more times repro'd it.
It's kind of surprising that this needs this
0001-Add-adversarial-ConditionalLockBuff to break. I suspect it's a question
of hint bits changing due to lazy_scan_noprune(), which then makes
HeapTupleHeaderIsHotUpdated() have a different return value, preventing the
"If the tuple is DEAD and doesn't chain to anything else"
path from being taken.
> We hit the "goto retry" on offnum 2, which is the first tuple with
> storage (you can see "the ghost" of the tuple from the LP_DEAD item at
> offnum 1, since the page isn't defragmented in pg_surgery). I think
> that this happens because the heap-only tuple at offnum 2 is fully
> DEAD to lazy_scan_prune, but hasn't been recognized as such by
> heap_page_prune. There is no way that they'll ever "agree" on the
> tuple being DEAD right now, because pruning still doesn't assume that
> an orphaned heap-only tuple is fully DEAD.
> We can either do that, or we can throw an error concerning corruption
> when heap_page_prune notices orphaned tuples. Neither seems
> particularly appealing. But it definitely makes no sense to allow
> lazy_scan_prune to spin in a futile attempt to reach agreement with
> heap_page_prune about a DEAD tuple really being DEAD.
Yea, this sucks. I think we should go for the rewrite of the
heap_prune_chain() logic. The current approach is just never going to be
robust.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:07 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 03:07 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 7:01 PM Andres Freund <[email protected]> wrote:
> > We can either do that, or we can throw an error concerning corruption
> > when heap_page_prune notices orphaned tuples. Neither seems
> > particularly appealing. But it definitely makes no sense to allow
> > lazy_scan_prune to spin in a futile attempt to reach agreement with
> > heap_page_prune about a DEAD tuple really being DEAD.
>
> Yea, this sucks. I think we should go for the rewrite of the
> heap_prune_chain() logic. The current approach is just never going to be
> robust.
No, it just isn't robust enough. But it's not that hard to fix. My
patch really wasn't invasive.
I confirmed that HeapTupleSatisfiesVacuum() and
heap_prune_satisfies_vacuum() agree that the heap-only tuple at offnum
2 is HEAPTUPLE_DEAD -- they are in agreement, as expected (so no
reason to think that there is a new bug involved). The problem here is
indeed just that heap_prune_chain() can't "get to" the tuple, given
its current design.
For anybody else that doesn't follow what we're talking about:
The "doesn't chain to anything else" code at the start of
heap_prune_chain() won't get to the heap-only tuple at offnum 2, since
the tuple is itself HeapTupleHeaderIsHotUpdated() -- the expectation
is that it'll be processed later on, once we locate the HOT chain's
root item. Since, of course, the "root item" was already LP_DEAD
before we even reached heap_page_prune() (on account of the pg_surgery
corruption), there is no possible way that that can happen later on.
And so we cannot find the same heap-only tuple and mark it LP_UNUSED
(which is how we always deal with HEAPTUPLE_DEAD heap-only tuples)
during pruning.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:18 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 03:18 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 7:01 PM Andres Freund <[email protected]> wrote:
> It's kind of surprising that this needs this
> 0001-Add-adversarial-ConditionalLockBuff to break. I suspect it's a question
> of hint bits changing due to lazy_scan_noprune(), which then makes
> HeapTupleHeaderIsHotUpdated() have a different return value, preventing the
> "If the tuple is DEAD and doesn't chain to anything else"
> path from being taken.
That makes sense as an explanation. Goes to show just how fragile the
"DEAD and doesn't chain to anything else" logic at the top of
heap_prune_chain really is.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:28 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2022-02-20 03:28 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-02-19 19:07:39 -0800, Peter Geoghegan wrote:
> On Sat, Feb 19, 2022 at 7:01 PM Andres Freund <[email protected]> wrote:
> > > We can either do that, or we can throw an error concerning corruption
> > > when heap_page_prune notices orphaned tuples. Neither seems
> > > particularly appealing. But it definitely makes no sense to allow
> > > lazy_scan_prune to spin in a futile attempt to reach agreement with
> > > heap_page_prune about a DEAD tuple really being DEAD.
> >
> > Yea, this sucks. I think we should go for the rewrite of the
> > heap_prune_chain() logic. The current approach is just never going to be
> > robust.
>
> No, it just isn't robust enough. But it's not that hard to fix. My
> patch really wasn't invasive.
I think we're in agreement there. We might think at some point about
backpatching too, but I'd rather have it stew in HEAD for a bit first.
> I confirmed that HeapTupleSatisfiesVacuum() and
> heap_prune_satisfies_vacuum() agree that the heap-only tuple at offnum
> 2 is HEAPTUPLE_DEAD -- they are in agreement, as expected (so no
> reason to think that there is a new bug involved). The problem here is
> indeed just that heap_prune_chain() can't "get to" the tuple, given
> its current design.
Right.
The reason that the "adversarial" patch makes a different is solely that it
changes the heap_surgery test to actually kill an item, which it doesn't
intend:
create temp table htab2(a int);
insert into htab2 values (100);
update htab2 set a = 200;
vacuum htab2;
-- redirected TIDs should be skipped
select heap_force_kill('htab2'::regclass, ARRAY['(0, 1)']::tid[]);
If the vacuum can get the cleanup lock due to the adversarial patch, the
heap_force_kill() doesn't do anything, because the first item is a
redirect. However if it *can't* get a cleanup lock, heap_force_kill() instead
targets the root item. Triggering the endless loop.
Hm. I think this might be a mild regression in 14. In < 14 we'd just skip the
tuple in lazy_scan_heap(), but now we have an uninterruptible endless
loop.
We'd do completely bogus stuff later in < 14 though, I think we'd just leave
it in place despite being older than relfrozenxid, which obviously has its own
set of issues.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:31 Peter Geoghegan <[email protected]>
parent: Peter Geoghegan <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 03:31 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 6:16 PM Peter Geoghegan <[email protected]> wrote:
> > Given that heap_surgery's raison d'etre is correcting corruption etc, I think
> > it makes sense for it to do as minimal work as possible. Iterating through a
> > HOT chain would be a problem if you e.g. tried to repair a page with HOT
> > corruption.
>
> I guess that's also true. There is at least a legitimate argument to
> be made for not leaving behind any orphaned heap-only tuples. The
> interface is a TID, and so the user may already believe that they're
> killing the heap-only, not just the root item (since ctid suggests
> that the TID of a heap-only tuple is the TID of the root item, which
> is kind of misleading).
Actually, I would say that heap_surgery's raison d'etre is making
weird errors related to corruption of this or that TID go away, so
that the user can cut their losses. That's how it's advertised.
Let's assume that we don't want to make VACUUM/pruning just treat
orphaned heap-only tuples as DEAD, regardless of their true HTSV-wise
status -- let's say that we want to err in the direction of doing
nothing at all with the page. Now we have to have a weird error in
VACUUM instead (not great, but better than just spinning between
lazy_scan_prune and heap_prune_page). And we've just created natural
demand for heap_surgery to deal with the problem by deleting whole HOT
chains (not just root items).
If we allow VACUUM to treat orphaned heap-only tuples as DEAD right
away, then we might as well do the same thing in heap_surgery, since
there is little chance that the user will get to the heap-only tuples
before VACUUM does (not something to rely on, at any rate).
Either way, I think we probably end up needing to teach heap_surgery
to kill entire HOT chains as a group, given a TID.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:40 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 03:40 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 7:28 PM Andres Freund <[email protected]> wrote:
> If the vacuum can get the cleanup lock due to the adversarial patch, the
> heap_force_kill() doesn't do anything, because the first item is a
> redirect. However if it *can't* get a cleanup lock, heap_force_kill() instead
> targets the root item. Triggering the endless loop.
But it shouldn't matter if the root item is an LP_REDIRECT or a normal
(not heap-only) tuple with storage. Either way it's the root of a HOT
chain.
The fact that pg_surgery treats LP_REDIRECT items differently from the
other kind of root items is just arbitrary. It seems to have more to
do with freezing tuples than killing tuples.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:47 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2022-02-20 03:47 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-02-19 19:31:21 -0800, Peter Geoghegan wrote:
> On Sat, Feb 19, 2022 at 6:16 PM Peter Geoghegan <[email protected]> wrote:
> > > Given that heap_surgery's raison d'etre is correcting corruption etc, I think
> > > it makes sense for it to do as minimal work as possible. Iterating through a
> > > HOT chain would be a problem if you e.g. tried to repair a page with HOT
> > > corruption.
> >
> > I guess that's also true. There is at least a legitimate argument to
> > be made for not leaving behind any orphaned heap-only tuples. The
> > interface is a TID, and so the user may already believe that they're
> > killing the heap-only, not just the root item (since ctid suggests
> > that the TID of a heap-only tuple is the TID of the root item, which
> > is kind of misleading).
>
> Actually, I would say that heap_surgery's raison d'etre is making
> weird errors related to corruption of this or that TID go away, so
> that the user can cut their losses. That's how it's advertised.
I'm not that sure those are that different... Imagine some corruption leading
to two hot chains ending in the same tid, which our fancy new secure pruning
algorithm might detect.
Either way, I'm a bit surprised about the logic to not allow killing redirect
items? What if you have a redirect pointing to an unused item?
> Let's assume that we don't want to make VACUUM/pruning just treat
> orphaned heap-only tuples as DEAD, regardless of their true HTSV-wise
> status
I don't think that'd ever be a good idea. Those tuples are visible to a
seqscan after all.
> -- let's say that we want to err in the direction of doing
> nothing at all with the page. Now we have to have a weird error in
> VACUUM instead (not great, but better than just spinning between
> lazy_scan_prune and heap_prune_page).
Non DEAD orphaned versions shouldn't cause a problem in lazy_scan_prune(). The
problem here is a DEAD orphaned HOT tuples, and those we should be able to
delete with the new page pruning logic, right?
I think it might be worth getting rid of the need for the retry approach by
reusing the same HTSV status array between heap_prune_page and
lazy_scan_prune. Then the only legitimate reason for seeing a DEAD item in
lazy_scan_prune() would be some form of corruption. And it'd be a pretty
decent performance boost, HTSV ain't cheap.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 03:56 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 03:56 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 7:47 PM Andres Freund <[email protected]> wrote:
> I'm not that sure those are that different... Imagine some corruption leading
> to two hot chains ending in the same tid, which our fancy new secure pruning
> algorithm might detect.
I suppose that's possible, but it doesn't seem all that likely to ever
happen, what with the xmin -> xmax cross-tuple matching stuff.
> Either way, I'm a bit surprised about the logic to not allow killing redirect
> items? What if you have a redirect pointing to an unused item?
Again, I simply think it boils down to having to treat HOT chains as a
whole unit when killing TIDs.
> > Let's assume that we don't want to make VACUUM/pruning just treat
> > orphaned heap-only tuples as DEAD, regardless of their true HTSV-wise
> > status
>
> I don't think that'd ever be a good idea. Those tuples are visible to a
> seqscan after all.
I agree (I don't hate it completely, but it seems mostly bad). This is
what leads me to the conclusion that pg_surgery has to be able to do
this instead. Surely it's not okay to have something that makes VACUUM
always end in error, that cannot even be fixed by pg_surgery.
> > -- let's say that we want to err in the direction of doing
> > nothing at all with the page. Now we have to have a weird error in
> > VACUUM instead (not great, but better than just spinning between
> > lazy_scan_prune and heap_prune_page).
>
> Non DEAD orphaned versions shouldn't cause a problem in lazy_scan_prune(). The
> problem here is a DEAD orphaned HOT tuples, and those we should be able to
> delete with the new page pruning logic, right?
Right. But what good does that really do? The problematic page had a
third tuple (at offnum 3) that was LIVE. If we could have done
something about the problematic tuple at offnum 2 (which is where we
got stuck), then we'd still be left with a very unpleasant choice
about what happens to the third tuple.
> I think it might be worth getting rid of the need for the retry approach by
> reusing the same HTSV status array between heap_prune_page and
> lazy_scan_prune. Then the only legitimate reason for seeing a DEAD item in
> lazy_scan_prune() would be some form of corruption. And it'd be a pretty
> decent performance boost, HTSV ain't cheap.
I guess it doesn't actually matter if we leave an aborted DEAD tuple
behind, that we could have pruned away, but didn't. The important
thing is to be consistent at the level of the page.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 04:21 Andres Freund <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2022-02-20 04:21 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On February 19, 2022 7:56:53 PM PST, Peter Geoghegan <[email protected]> wrote:
>On Sat, Feb 19, 2022 at 7:47 PM Andres Freund <[email protected]> wrote:
>> Non DEAD orphaned versions shouldn't cause a problem in lazy_scan_prune(). The
>> problem here is a DEAD orphaned HOT tuples, and those we should be able to
>> delete with the new page pruning logic, right?
>
>Right. But what good does that really do? The problematic page had a
>third tuple (at offnum 3) that was LIVE. If we could have done
>something about the problematic tuple at offnum 2 (which is where we
>got stuck), then we'd still be left with a very unpleasant choice
>about what happens to the third tuple.
Why does anything need to happen to it from vacuum's POV? It'll not be a problem for freezing etc. Until it's deleted vacuum doesn't need to care.
Probably worth a WARNING, and amcheck definitely needs to detect it, but otherwise I think it's fine to just continue.
>> I think it might be worth getting rid of the need for the retry approach by
>> reusing the same HTSV status array between heap_prune_page and
>> lazy_scan_prune. Then the only legitimate reason for seeing a DEAD item in
>> lazy_scan_prune() would be some form of corruption. And it'd be a pretty
>> decent performance boost, HTSV ain't cheap.
>
>I guess it doesn't actually matter if we leave an aborted DEAD tuple
>behind, that we could have pruned away, but didn't. The important
>thing is to be consistent at the level of the page.
That's not ok, because it opens up dangers of being interpreted differently after wraparound etc.
But I don't see any cases where it would happen with the new pruning logic in your patch and sharing the HTSV status array?
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 04:28 Peter Geoghegan <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Peter Geoghegan @ 2022-02-20 04:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 8:21 PM Andres Freund <[email protected]> wrote:
> Why does anything need to happen to it from vacuum's POV? It'll not be a problem for freezing etc. Until it's deleted vacuum doesn't need to care.
>
> Probably worth a WARNING, and amcheck definitely needs to detect it, but otherwise I think it's fine to just continue.
Maybe that's true, but it's just really weird to imagine not having an
LP_REDIRECT that points to the LIVE item here, without throwing an
error. Seems kind of iffy, to say the least.
> >I guess it doesn't actually matter if we leave an aborted DEAD tuple
> >behind, that we could have pruned away, but didn't. The important
> >thing is to be consistent at the level of the page.
>
> That's not ok, because it opens up dangers of being interpreted differently after wraparound etc.
>
> But I don't see any cases where it would happen with the new pruning logic in your patch and sharing the HTSV status array?
Right. Fundamentally, there isn't any reason why it should matter that
VACUUM reached the heap page just before (rather than concurrent with
or just after) some xact that inserted or updated on the page aborts.
Just as long as we have a consistent idea about what's going on at the
level of the whole page (or maybe the level of each HOT chain, but the
whole page level seems simpler to me).
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
@ 2022-02-20 15:03 Robert Haas <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Robert Haas @ 2022-02-20 15:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Masahiko Sawada <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Feb 19, 2022 at 8:54 PM Andres Freund <[email protected]> wrote:
> > Leaving behind disconnected/orphaned heap-only tuples is pretty much
> > pointless anyway, since they'll never be accessible by index scans.
> > Even after a REINDEX, since there is no root item from the heap page
> > to go in the index. (A dump and restore might work better, though.)
>
> Given that heap_surgery's raison d'etre is correcting corruption etc, I think
> it makes sense for it to do as minimal work as possible. Iterating through a
> HOT chain would be a problem if you e.g. tried to repair a page with HOT
> corruption.
Yeah, I agree. I don't have time to respond to all of these emails
thoroughly right now, but I think it's really important that
pg_surgery do the exact surgery the user requested, and not any other
work. I don't think that page defragmentation should EVER be REQUIRED
as a condition of other work. If other code is relying on that, I'd
say it's busted. I'm a little more uncertain about the case where we
kill the root tuple of a HOT chain, because I can see that this might
leave the page a state where sequential scans see the tuple at the end
of the chain and index scans don't. I'm not sure whether that should
be the responsibility of pg_surgery itself to avoid, or whether that's
your problem as a user of it -- although I lean mildly toward the
latter view, at the moment. But in any case surely the pruning code
can't just decide to go into an infinite loop if that happens. Code
that manipulates the states of data pages needs to be as robust
against arbitrary on-disk states as we can reasonably make it, because
pages get garbled on disk all the time.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2022-02-20 15:03 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-04-04 05:48 [PATCH v1] Fine tune documentation for tableam Justin Pryzby <[email protected]>
2022-02-18 22:11 Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-19 01:00 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-19 23:08 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 00:12 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 00:22 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 01:22 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 01:54 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 02:16 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 03:01 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 03:07 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 03:28 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 03:40 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 03:18 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 03:31 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 03:47 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 03:56 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 04:21 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Andres Freund <[email protected]>
2022-02-20 04:28 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Peter Geoghegan <[email protected]>
2022-02-20 15:03 ` Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox