public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v8 1/8] pg_dump: make CLUSTER ON a separate dump object..
4+ messages / 3 participants
[nested] [flat]
* [PATCH v8 1/8] pg_dump: make CLUSTER ON a separate dump object..
@ 2020-11-26 20:37 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw)
..since it needs to be restored after any child indexes are restored *and
attached*. The order needs to be:
1) restore child and parent index (order doesn't matter);
2) attach child index;
3) set cluster on child and parent index (order doesn't matter);
---
src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++--------
src/bin/pg_dump/pg_dump.h | 8 ++++
src/bin/pg_dump/pg_dump_sort.c | 8 ++++
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index eb988d7eb4..e93d2eb828 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, const TableInfo *tbinfo);
static void dumpSequenceData(Archive *fout, const TableDataInfo *tdinfo);
static void dumpIndex(Archive *fout, const IndxInfo *indxinfo);
static void dumpIndexAttach(Archive *fout, const IndexAttachInfo *attachinfo);
+static void dumpIndexClusterOn(Archive *fout, const IndexClusterInfo *clusterinfo);
static void dumpStatisticsExt(Archive *fout, const StatsExtInfo *statsextinfo);
static void dumpConstraint(Archive *fout, const ConstraintInfo *coninfo);
static void dumpTableConstraintComment(Archive *fout, const ConstraintInfo *coninfo);
@@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_inddependcollversions;
int ntups;
+ int ncluster = 0;
+ IndexClusterInfo *clusterinfo;
+ clusterinfo = (IndexClusterInfo *)
+ pg_malloc0(numTables * sizeof(IndexClusterInfo));
+
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &tblinfo[i];
@@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
/* Plain secondary index */
indxinfo[j].indexconstraint = 0;
}
+
+ /* Record each table's CLUSTERed index, if any */
+ if (indxinfo[j].indisclustered)
+ {
+ IndxInfo *index = &indxinfo[j];
+ IndexClusterInfo *cluster = &clusterinfo[ncluster];
+
+ cluster->dobj.objType = DO_INDEX_CLUSTER_ON;
+ cluster->dobj.catId.tableoid = 0;
+ cluster->dobj.catId.oid = 0;
+ AssignDumpId(&cluster->dobj);
+ cluster->dobj.name = pg_strdup(index->dobj.name);
+ cluster->dobj.namespace = index->indextable->dobj.namespace;
+ cluster->index = index;
+ cluster->indextable = &tblinfo[i];
+
+ /* The CLUSTER ON depends on its index.. */
+ addObjectDependency(&cluster->dobj, index->dobj.dumpId);
+
+ ncluster++;
+ }
}
PQclear(res);
@@ -10323,6 +10350,9 @@ dumpDumpableObject(Archive *fout, const DumpableObject *dobj)
case DO_SUBSCRIPTION:
dumpSubscription(fout, (const SubscriptionInfo *) dobj);
break;
+ case DO_INDEX_CLUSTER_ON:
+ dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj);
+ break;
case DO_PRE_DATA_BOUNDARY:
case DO_POST_DATA_BOUNDARY:
/* never dumped, nothing to do */
@@ -16543,6 +16573,41 @@ getAttrName(int attrnum, const TableInfo *tblInfo)
return NULL; /* keep compiler quiet */
}
+/*
+ * dumpIndexClusterOn
+ * record that the index is clustered.
+ */
+static void
+dumpIndexClusterOn(Archive *fout, const IndexClusterInfo *clusterinfo)
+{
+ DumpOptions *dopt = fout->dopt;
+ TableInfo *tbinfo = clusterinfo->indextable;
+ char *qindxname;
+ PQExpBuffer q;
+
+ if (dopt->dataOnly)
+ return;
+
+ q = createPQExpBuffer();
+ qindxname = pg_strdup(fmtId(clusterinfo->dobj.name));
+
+ /* index name is not qualified in this syntax */
+ appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n",
+ fmtQualifiedDumpable(tbinfo), qindxname);
+
+ if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
+ ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId,
+ ARCHIVE_OPTS(.tag = clusterinfo->dobj.name,
+ .namespace = tbinfo->dobj.namespace->dobj.name,
+ .owner = tbinfo->rolname,
+ .description = "INDEX CLUSTER ON",
+ .section = SECTION_POST_DATA,
+ .createStmt = q->data));
+
+ destroyPQExpBuffer(q);
+ free(qindxname);
+}
+
/*
* dumpIndex
* write out to fout a user-defined index
@@ -16597,16 +16662,6 @@ dumpIndex(Archive *fout, const IndxInfo *indxinfo)
* similar code in dumpConstraint!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- qindxname);
- }
-
/*
* If the index has any statistics on some of its columns, generate
* the associated ALTER INDEX queries.
@@ -16933,16 +16988,6 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
* similar code in dumpIndex!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- fmtId(indxinfo->dobj.name));
- }
-
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
@@ -18448,6 +18493,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
break;
case DO_INDEX:
case DO_INDEX_ATTACH:
+ case DO_INDEX_CLUSTER_ON:
case DO_STATSEXT:
case DO_REFRESH_MATVIEW:
case DO_TRIGGER:
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 0a2213fb06..627c8fbdab 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -54,6 +54,7 @@ typedef enum
DO_ATTRDEF,
DO_INDEX,
DO_INDEX_ATTACH,
+ DO_INDEX_CLUSTER_ON,
DO_STATSEXT,
DO_RULE,
DO_TRIGGER,
@@ -386,6 +387,13 @@ typedef struct _indxInfo
DumpId indexconstraint;
} IndxInfo;
+typedef struct _indexClusterInfo
+{
+ DumpableObject dobj;
+ TableInfo *indextable; /* link to table the index is for */
+ IndxInfo *index; /* link to index itself */
+} IndexClusterInfo;
+
typedef struct _indexAttachInfo
{
DumpableObject dobj;
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 46461fb6a1..dd5b233196 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -75,6 +75,7 @@ enum dbObjectTypePriorities
PRIO_CONSTRAINT,
PRIO_INDEX,
PRIO_INDEX_ATTACH,
+ PRIO_INDEX_CLUSTER_ON,
PRIO_STATSEXT,
PRIO_RULE,
PRIO_TRIGGER,
@@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] =
PRIO_ATTRDEF, /* DO_ATTRDEF */
PRIO_INDEX, /* DO_INDEX */
PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */
+ PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */
PRIO_STATSEXT, /* DO_STATSEXT */
PRIO_RULE, /* DO_RULE */
PRIO_TRIGGER, /* DO_TRIGGER */
@@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] =
PRIO_PUBLICATION, /* DO_PUBLICATION */
PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */
PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */
+
};
StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1),
@@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)
"INDEX ATTACH %s (ID %d)",
obj->name, obj->dumpId);
return;
+ case DO_INDEX_CLUSTER_ON:
+ snprintf(buf, bufsize,
+ "INDEX CLUSTER ON %s (ID %d)",
+ obj->name, obj->dumpId);
+ return;
case DO_STATSEXT:
snprintf(buf, bufsize,
"STATISTICS %s (ID %d OID %u)",
--
2.17.0
--O5XBE6gyVG5Rl6Rj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0002-Implement-CLUSTER-of-partitioned-table.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* [BUG] Panic due to incorrect missingContrecPtr after promotion
@ 2022-02-22 19:20 Imseih (AWS), Sami <[email protected]>
2022-05-26 23:53 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Imseih (AWS), Sami @ 2022-02-22 19:20 UTC (permalink / raw)
To: pgsql-hackers
On 13.5 a wal flush PANIC is encountered after a standby is promoted.
With debugging, it was found that when a standby skips a missing continuation record on recovery, the missingContrecPtr is not invalidated after the record is skipped. Therefore, when the standby is promoted to a primary it writes an overwrite_contrecord with an LSN of the missingContrecPtr, which is now in the past. On flush time, this causes a PANIC. From what I can see, this failure scenario can only occur after a standby is promoted.
The overwrite_contrecord was introduced in 13.5 with https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ff9f111bce24.
Attached is a patch and a TAP test to handle this condition. The patch ensures that an overwrite_contrecord is only created if the missingContrecPtr is ahead of the last wal record.
To reproduce:
Run the new tap test recovery/t/029_overwrite_contrecord_promotion.pl without the attached patch
2022-02-22 18:38:15.526 UTC [31138] LOG: started streaming WAL from primary at 0/2000000 on timeline 1
2022-02-22 18:38:15.535 UTC [31105] LOG: successfully skipped missing contrecord at 0/1FFC620, overwritten at 2022-02-22 18:38:15.136482+00
2022-02-22 18:38:15.535 UTC [31105] CONTEXT: WAL redo at 0/2000028 for XLOG/OVERWRITE_CONTRECORD: lsn 0/1FFC620; time 2022-02-22 18:38:15.136482+00
…
…..
2022-02-22 18:38:15.575 UTC [31103] PANIC: xlog flush request 0/201EC70 is not satisfied --- flushed only to 0/2000088
2022-02-22 18:38:15.575 UTC [31101] LOG: checkpointer process (PID 31103) was terminated by signal 6: Aborted
….
…..
With the patch, running the same tap test succeeds and a PANIC is not observed.
Thanks
Sami Imseih
Amazon Web Services
Attachments:
[application/octet-stream] 0001-Fix-missing-continuation-record-after-standby-promot.patch (6.6K, ../../[email protected]/3-0001-Fix-missing-continuation-record-after-standby-promot.patch)
download | inline diff:
From e7c2e425eba642c8e9c379c5fecc4bd5caf28997 Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Tue, 22 Feb 2022 19:09:36 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion
Fix a condition where a recently promoted standby attempts to
write an OVERWRITE_RECORD with an LSN of the previously read
aborted record.
---
src/backend/access/transam/xlog.c | 16 ++-
...inuation-record-after-standby-promot.patch | 134 ++++++++++++++++++
2 files changed, 149 insertions(+), 1 deletion(-)
create mode 100644 src/test/recovery/0001-Fix-missing-continuation-record-after-standby-promot.patch
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0d2bd7a357..56c2fdec96 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5423,11 +5423,25 @@ StartupXLOG(void)
* made it through and start writing after the portion that persisted.
* (It's critical to first write an OVERWRITE_CONTRECORD message, which
* we'll do as soon as we're open for writing new WAL.)
+ *
+ * If the last wal record is ahead of the missing contrecord, this is
+ * a recently promoted primary and we should not write an overwrite
+ * contrecord.
*/
if (!XLogRecPtrIsInvalid(missingContrecPtr))
{
Assert(!XLogRecPtrIsInvalid(abortedRecPtr));
- EndOfLog = missingContrecPtr;
+ if (endOfRecoveryInfo->lastRec < missingContrecPtr)
+ {
+ elog(DEBUG2, "setting end of wal to missing continuation record %X/%X",
+ LSN_FORMAT_ARGS(missingContrecPtr));
+ EndOfLog = missingContrecPtr;
+ }
+ else
+ {
+ elog(DEBUG2, "resetting aborted record");
+ abortedRecPtr = InvalidXLogRecPtr;
+ }
}
/*
diff --git a/src/test/recovery/0001-Fix-missing-continuation-record-after-standby-promot.patch b/src/test/recovery/0001-Fix-missing-continuation-record-after-standby-promot.patch
new file mode 100644
index 0000000000..40d922801b
--- /dev/null
+++ b/src/test/recovery/0001-Fix-missing-continuation-record-after-standby-promot.patch
@@ -0,0 +1,134 @@
+From cb344355facb3e6f793013b0b9998683277f3bd8 Mon Sep 17 00:00:00 2001
+From: "Sami Imseih (AWS)" <[email protected]>
+Date: Tue, 22 Feb 2022 18:59:44 +0000
+Subject: [PATCH 1/1] Fix "missing continuation record" after standby
+ promotion.
+
+Fix a condition where a recently promoted standby attempts to
+write an OVERWRITE_RECORD with an LSN of the previously read
+aborted record.
+---
+ .../t/029_overwrite_contrecord_promotion.pl | 111 ++++++++++++++++++
+ 1 file changed, 111 insertions(+)
+ create mode 100644 src/test/recovery/t/029_overwrite_contrecord_promotion.pl
+
+diff --git a/src/test/recovery/t/029_overwrite_contrecord_promotion.pl b/src/test/recovery/t/029_overwrite_contrecord_promotion.pl
+new file mode 100644
+index 0000000000..ea4ebb32c0
+--- /dev/null
++++ b/src/test/recovery/t/029_overwrite_contrecord_promotion.pl
+@@ -0,0 +1,111 @@
++# Copyright (c) 2021-2022, PostgreSQL Global Development Group
++
++# Tests for resetting the "aborted record" after a promotion.
++
++use strict;
++use warnings;
++
++use FindBin;
++use PostgreSQL::Test::Cluster;
++use PostgreSQL::Test::Utils;
++use Test::More;
++
++# Test: Create a physical replica that's missing the last WAL file,
++# then restart the primary to create a divergent WAL file and observe
++# that the replica resets the "aborted record" after a promotion.
++
++my $node = PostgreSQL::Test::Cluster->new('primary');
++$node->init(allows_streaming => 1);
++# We need these settings for stability of WAL behavior.
++$node->append_conf(
++ 'postgresql.conf', qq(
++autovacuum = off
++wal_keep_size = 1GB
++log_min_messages = DEBUG2
++));
++$node->start;
++
++$node->safe_psql('postgres', 'create table filler (a int, b text)');
++
++# Now consume all remaining room in the current WAL segment, leaving
++# space enough only for the start of a largish record.
++$node->safe_psql(
++ 'postgres', q{
++DO $$
++DECLARE
++ wal_segsize int := setting::int FROM pg_settings WHERE name = 'wal_segment_size';
++ remain int;
++ iters int := 0;
++BEGIN
++ LOOP
++ INSERT into filler
++ select g, repeat(md5(g::text), (random() * 60 + 1)::int)
++ from generate_series(1, 10) g;
++
++ remain := wal_segsize - (pg_current_wal_insert_lsn() - '0/0') % wal_segsize;
++ IF remain < 2 * setting::int from pg_settings where name = 'block_size' THEN
++ RAISE log 'exiting after % iterations, % bytes to end of WAL segment', iters, remain;
++ EXIT;
++ END IF;
++ iters := iters + 1;
++ END LOOP;
++END
++$$;
++});
++
++my $initfile = $node->safe_psql('postgres',
++ 'SELECT pg_walfile_name(pg_current_wal_insert_lsn())');
++$node->safe_psql('postgres',
++ qq{SELECT pg_logical_emit_message(true, 'test 026', repeat('xyzxz', 123456))}
++);
++#$node->safe_psql('postgres', qq{create table foo ()});
++my $endfile = $node->safe_psql('postgres',
++ 'SELECT pg_walfile_name(pg_current_wal_insert_lsn())');
++ok($initfile ne $endfile, "$initfile differs from $endfile");
++
++# Now stop abruptly, to avoid a stop checkpoint. We can remove the tail file
++# afterwards, and on startup the large message should be overwritten with new
++# contents
++$node->stop('immediate');
++
++unlink $node->basedir . "/pgdata/pg_wal/$endfile"
++ or die "could not unlink " . $node->basedir . "/pgdata/pg_wal/$endfile: $!";
++
++# OK, create a standby at this spot.
++$node->backup_fs_cold('backup');
++my $node_standby = PostgreSQL::Test::Cluster->new('standby');
++$node_standby->init_from_backup($node, 'backup', has_streaming => 1);
++
++$node_standby->start;
++$node->start;
++
++$node->safe_psql('postgres',
++ qq{create table foo (a text); insert into foo values ('hello')});
++$node->safe_psql('postgres',
++ qq{SELECT pg_logical_emit_message(true, 'test 026', 'AABBCC')});
++
++my $until_lsn = $node->safe_psql('postgres', "SELECT pg_current_wal_lsn()");
++my $caughtup_query =
++ "SELECT '$until_lsn'::pg_lsn <= pg_last_wal_replay_lsn()";
++$node_standby->poll_query_until('postgres', $caughtup_query)
++ or die "Timed out while waiting for standby to catch up";
++
++ok($node_standby->safe_psql('postgres', 'select * from foo') eq 'hello',
++ 'standby replays past overwritten contrecord');
++
++$ENV{PGDATA} = $node_standby->data_dir;
++$ENV{PGPORT} = $node_standby->port;
++$ENV{PGGHOST} = $node_standby->host;
++system "psql -c 'select pg_promote()'";
++
++# Verify message appears in standby's log
++my $log = slurp_file($node_standby->logfile);
++like(
++ $log,
++ qr[resetting aborted record],
++ "found log line in standby");
++
++$node->stop;
++$node_standby->stop;
++
++done_testing();
+--
+2.32.0
+
--
2.32.0
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
2022-02-22 19:20 [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-05-26 23:53 ` Michael Paquier <[email protected]>
2022-05-27 00:03 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Michael Paquier @ 2022-05-26 23:53 UTC (permalink / raw)
To: Imseih (AWS), Sami <[email protected]>; +Cc: pgsql-hackers
On Tue, Feb 22, 2022 at 07:20:55PM +0000, Imseih (AWS), Sami wrote:
> The overwrite_contrecord was introduced in 13.5 with https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ff9f111bce24.
>
> Attached is a patch and a TAP test to handle this condition. The
> patch ensures that an overwrite_contrecord is only created if the
> missingContrecPtr is ahead of the last wal record.
The test you are introducing to force a complete segment to be filled
is funky, and kind of nice actually while being cheap. This part
particularly makes the test predictable:
++unlink $node->basedir . "/pgdata/pg_wal/$endfile"
++ or die "could not unlink " . $node->basedir
. "/pgdata/pg_wal/$endfile: $!";
I really like that.
> With the patch, running the same tap test succeeds and a PANIC is
> not observed.
This needs a very close lookup, I'll try to check all that except if
somebody beats me to it.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
2022-02-22 19:20 [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-05-26 23:53 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
@ 2022-05-27 00:03 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Michael Paquier @ 2022-05-27 00:03 UTC (permalink / raw)
To: Imseih (AWS), Sami <[email protected]>; +Cc: pgsql-hackers
On Fri, May 27, 2022 at 08:53:03AM +0900, Michael Paquier wrote:
> This needs a very close lookup, I'll try to check all that except if
> somebody beats me to it.
Please ignore that.. I need more coffee, and likely a break.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-05-27 00:03 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 20:37 [PATCH v8 1/8] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]>
2022-02-22 19:20 [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-05-26 23:53 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
2022-05-27 00:03 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[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