public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 3/8] Add rewrite rules and tupdesc flags
34+ messages / 8 participants
[nested] [flat]

* [PATCH 3/8] Add rewrite rules and tupdesc flags
@ 2018-06-18 12:34 Ildus Kurbangaliev <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Ildus Kurbangaliev @ 2018-06-18 12:34 UTC (permalink / raw)

Signed-off-by: Ildus Kurbangaliev <[email protected]>
---
 src/backend/access/brin/brin_tuple.c   |  2 +
 src/backend/access/common/heaptuple.c  | 27 +++++++++-
 src/backend/access/common/indextuple.c |  2 +
 src/backend/access/common/tupdesc.c    | 20 ++++++++
 src/backend/access/heap/heapam.c       | 19 ++++---
 src/backend/access/heap/tuptoaster.c   |  2 +
 src/backend/commands/copy.c            |  2 +-
 src/backend/commands/createas.c        |  2 +-
 src/backend/commands/matview.c         |  2 +-
 src/backend/commands/tablecmds.c       | 69 ++++++++++++++++++++++++--
 src/backend/utils/adt/expandedrecord.c |  1 +
 src/backend/utils/cache/relcache.c     |  4 ++
 src/include/access/heapam.h            |  3 +-
 src/include/access/hio.h               |  2 +
 src/include/access/htup_details.h      |  9 +++-
 src/include/access/tupdesc.h           |  7 +++
 src/include/access/tuptoaster.h        |  1 -
 src/include/commands/event_trigger.h   |  1 +
 18 files changed, 157 insertions(+), 18 deletions(-)

diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index a5bc6f5749..df391bcc78 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -96,6 +96,7 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
 	int			keyno;
 	int			idxattno;
 	uint16		phony_infomask = 0;
+	uint16		phony_infomask2 = 0;
 	bits8	   *phony_nullbitmap;
 	Size		len,
 				hoff,
@@ -187,6 +188,7 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
 					(char *) rettuple + hoff,
 					data_len,
 					&phony_infomask,
+					&phony_infomask2,
 					phony_nullbitmap);
 
 	/* done with these */
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 783b04a3cb..b05dee6195 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -177,6 +177,7 @@ fill_val(Form_pg_attribute att,
 		 int *bitmask,
 		 char **dataP,
 		 uint16 *infomask,
+		 uint16 *infomask2,
 		 Datum datum,
 		 bool isnull)
 {
@@ -207,6 +208,9 @@ fill_val(Form_pg_attribute att,
 		**bit |= *bitmask;
 	}
 
+	if (OidIsValid(att->attcompression))
+		*infomask2 |= HEAP_HASCUSTOMCOMPRESSED;
+
 	/*
 	 * XXX we use the att_align macros on the pointer value itself, not on an
 	 * offset.  This is a bit of a hack.
@@ -245,6 +249,15 @@ fill_val(Form_pg_attribute att,
 				/* no alignment, since it's short by definition */
 				data_length = VARSIZE_EXTERNAL(val);
 				memcpy(data, val, data_length);
+
+				if (VARATT_IS_EXTERNAL_ONDISK(val))
+				{
+					struct varatt_external toast_pointer;
+
+					VARATT_EXTERNAL_GET_POINTER(toast_pointer, val);
+					if (VARATT_EXTERNAL_IS_CUSTOM_COMPRESSED(toast_pointer))
+						*infomask2 |= HEAP_HASCUSTOMCOMPRESSED;
+				}
 			}
 		}
 		else if (VARATT_IS_SHORT(val))
@@ -268,6 +281,9 @@ fill_val(Form_pg_attribute att,
 											  att->attalign);
 			data_length = VARSIZE(val);
 			memcpy(data, val, data_length);
+
+			if (VARATT_IS_CUSTOM_COMPRESSED(val))
+				*infomask2 |= HEAP_HASCUSTOMCOMPRESSED;
 		}
 	}
 	else if (att->attlen == -2)
@@ -304,7 +320,7 @@ void
 heap_fill_tuple(TupleDesc tupleDesc,
 				Datum *values, bool *isnull,
 				char *data, Size data_size,
-				uint16 *infomask, bits8 *bit)
+				uint16 *infomask, uint16 *infomask2, bits8 *bit)
 {
 	bits8	   *bitP;
 	int			bitmask;
@@ -328,6 +344,7 @@ heap_fill_tuple(TupleDesc tupleDesc,
 	}
 
 	*infomask &= ~(HEAP_HASNULL | HEAP_HASVARWIDTH | HEAP_HASEXTERNAL);
+	*infomask2 &= ~HEAP_HASCUSTOMCOMPRESSED;
 
 	for (i = 0; i < numberOfAttributes; i++)
 	{
@@ -338,6 +355,7 @@ heap_fill_tuple(TupleDesc tupleDesc,
 				 &bitmask,
 				 &data,
 				 infomask,
+				 infomask2,
 				 values ? values[i] : PointerGetDatum(NULL),
 				 isnull ? isnull[i] : true);
 	}
@@ -752,6 +770,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
 	int			bitMask = 0;
 	char	   *targetData;
 	uint16	   *infoMask;
+	uint16	   *infoMask2;
 
 	Assert((targetHeapTuple && !targetMinimalTuple)
 		   || (!targetHeapTuple && targetMinimalTuple));
@@ -864,6 +883,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
 								  + offsetof(HeapTupleHeaderData, t_bits));
 		targetData = (char *) (*targetHeapTuple)->t_data + hoff;
 		infoMask = &(targetTHeader->t_infomask);
+		infoMask2 = &(targetTHeader->t_infomask2);
 	}
 	else
 	{
@@ -882,6 +902,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
 								  + offsetof(MinimalTupleData, t_bits));
 		targetData = (char *) *targetMinimalTuple + hoff;
 		infoMask = &((*targetMinimalTuple)->t_infomask);
+		infoMask2 = &((*targetMinimalTuple)->t_infomask2);
 	}
 
 	if (targetNullLen > 0)
@@ -934,6 +955,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
 					 &bitMask,
 					 &targetData,
 					 infoMask,
+					 infoMask2,
 					 attrmiss[attnum].am_value,
 					 false);
 		}
@@ -944,6 +966,7 @@ expand_tuple(HeapTuple *targetHeapTuple,
 					 &bitMask,
 					 &targetData,
 					 infoMask,
+					 infoMask2,
 					 (Datum) 0,
 					 true);
 		}
@@ -1093,6 +1116,7 @@ heap_form_tuple(TupleDesc tupleDescriptor,
 					(char *) td + hoff,
 					data_len,
 					&td->t_infomask,
+					&td->t_infomask2,
 					(hasnull ? td->t_bits : NULL));
 
 	return tuple;
@@ -1415,6 +1439,7 @@ heap_form_minimal_tuple(TupleDesc tupleDescriptor,
 					(char *) tuple + hoff,
 					data_len,
 					&tuple->t_infomask,
+					&tuple->t_infomask2,
 					(hasnull ? tuple->t_bits : NULL));
 
 	return tuple;
diff --git a/src/backend/access/common/indextuple.c b/src/backend/access/common/indextuple.c
index 77c742491f..0c73bac69b 100644
--- a/src/backend/access/common/indextuple.c
+++ b/src/backend/access/common/indextuple.c
@@ -50,6 +50,7 @@ index_form_tuple(TupleDesc tupleDescriptor,
 	unsigned short infomask = 0;
 	bool		hasnull = false;
 	uint16		tupmask = 0;
+	uint16		tupmask2 = 0;
 	int			numberOfAttributes = tupleDescriptor->natts;
 
 #ifdef TOAST_INDEX_HACK
@@ -162,6 +163,7 @@ index_form_tuple(TupleDesc tupleDescriptor,
 					(char *) tp + hoff,
 					data_size,
 					&tupmask,
+					&tupmask2,
 					(hasnull ? (bits8 *) tp + sizeof(IndexTupleData) : NULL));
 
 #ifdef TOAST_INDEX_HACK
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 0158950a43..cb344b55aa 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -20,6 +20,7 @@
 #include "postgres.h"
 
 #include "access/htup_details.h"
+#include "access/reloptions.h"
 #include "access/tupdesc_details.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
@@ -72,6 +73,7 @@ CreateTemplateTupleDesc(int natts)
 	desc->constr = NULL;
 	desc->tdtypeid = RECORDOID;
 	desc->tdtypmod = -1;
+	desc->tdflags = 0;
 	desc->tdrefcount = -1;		/* assume not reference-counted */
 
 	return desc;
@@ -94,8 +96,17 @@ CreateTupleDesc(int natts, Form_pg_attribute *attrs)
 	desc = CreateTemplateTupleDesc(natts);
 
 	for (i = 0; i < natts; ++i)
+	{
 		memcpy(TupleDescAttr(desc, i), attrs[i], ATTRIBUTE_FIXED_PART_SIZE);
 
+		/*
+		 * If even one of attributes is compressed we save information about
+		 * it to TupleDesc flags
+		 */
+		if (OidIsValid(attrs[i]->attcompression))
+			desc->tdflags |= TD_ATTR_CUSTOM_COMPRESSED;
+	}
+
 	return desc;
 }
 
@@ -136,6 +147,7 @@ CreateTupleDescCopy(TupleDesc tupdesc)
 	/* We can copy the tuple type identification, too */
 	desc->tdtypeid = tupdesc->tdtypeid;
 	desc->tdtypmod = tupdesc->tdtypmod;
+	desc->tdflags = tupdesc->tdflags;
 
 	return desc;
 }
@@ -215,6 +227,7 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
 	/* We can copy the tuple type identification, too */
 	desc->tdtypeid = tupdesc->tdtypeid;
 	desc->tdtypmod = tupdesc->tdtypmod;
+	desc->tdflags = tupdesc->tdflags;
 
 	return desc;
 }
@@ -300,6 +313,7 @@ TupleDescCopyEntry(TupleDesc dst, AttrNumber dstAttno,
 	dstAtt->atthasdef = false;
 	dstAtt->atthasmissing = false;
 	dstAtt->attidentity = '\0';
+	dstAtt->attcompression = InvalidOid;
 }
 
 /*
@@ -414,6 +428,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 		return false;
 	if (tupdesc1->tdtypeid != tupdesc2->tdtypeid)
 		return false;
+	if (tupdesc1->tdflags != tupdesc2->tdflags)
+		return false;
 
 	for (i = 0; i < tupdesc1->natts; i++)
 	{
@@ -464,6 +480,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 			return false;
 		if (attr1->attcollation != attr2->attcollation)
 			return false;
+		if (attr1->attcompression != attr2->attcompression)
+			return false;
 		/* attacl, attoptions and attfdwoptions are not even present... */
 	}
 
@@ -549,6 +567,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 	}
 	else if (tupdesc2->constr != NULL)
 		return false;
+
 	return true;
 }
 
@@ -654,6 +673,7 @@ TupleDescInitEntry(TupleDesc desc,
 	att->attalign = typeForm->typalign;
 	att->attstorage = typeForm->typstorage;
 	att->attcollation = typeForm->typcollation;
+	att->attcompression = InvalidOid;
 
 	ReleaseSysCache(tuple);
 }
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 1ecc16888e..34dd671430 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -70,7 +70,8 @@
 
 
 static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
-					TransactionId xid, CommandId cid, int options);
+					TransactionId xid, CommandId cid, int options,
+					BulkInsertState bistate);
 static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
 				Buffer newbuf, HeapTuple oldtup,
 				HeapTuple newtup, HeapTuple old_key_tup,
@@ -1846,13 +1847,14 @@ UpdateXmaxHintBits(HeapTupleHeader tuple, Buffer buffer, TransactionId xid)
  * GetBulkInsertState - prepare status object for a bulk insert
  */
 BulkInsertState
-GetBulkInsertState(void)
+GetBulkInsertState(HTAB *preserved_am_info)
 {
 	BulkInsertState bistate;
 
 	bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData));
 	bistate->strategy = GetAccessStrategy(BAS_BULKWRITE);
 	bistate->current_buf = InvalidBuffer;
+	bistate->preserved_am_info = preserved_am_info;
 	return bistate;
 }
 
@@ -1942,7 +1944,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	 * Note: below this point, heaptup is the data we actually intend to store
 	 * into the relation; tup is the caller's original untoasted data.
 	 */
-	heaptup = heap_prepare_insert(relation, tup, xid, cid, options);
+	heaptup = heap_prepare_insert(relation, tup, xid, cid, options, bistate);
 
 	/*
 	 * Find buffer to insert this tuple into.  If the page is all visible,
@@ -2109,7 +2111,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
  */
 static HeapTuple
 heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
-					CommandId cid, int options)
+					CommandId cid, int options, BulkInsertState bistate)
 {
 	/*
 	 * Parallel operations are required to be strictly read-only in a parallel
@@ -2147,8 +2149,11 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
 		return tup;
 	}
 	else if (HeapTupleHasExternal(tup)
+			 || RelationGetDescr(relation)->tdflags & TD_ATTR_CUSTOM_COMPRESSED
+			 || HeapTupleHasCustomCompressed(tup)
 			 || tup->t_len > TOAST_TUPLE_THRESHOLD)
-		return toast_insert_or_update(relation, tup, NULL, options, NULL);
+		return toast_insert_or_update(relation, tup, NULL, options,
+									  bistate ? bistate->preserved_am_info : NULL);
 	else
 		return tup;
 }
@@ -2190,7 +2195,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 	heaptuples = palloc(ntuples * sizeof(HeapTuple));
 	for (i = 0; i < ntuples; i++)
 		heaptuples[i] = heap_prepare_insert(relation, tuples[i],
-											xid, cid, options);
+											xid, cid, options, bistate);
 
 	/*
 	 * We're about to do the actual inserts -- but check for conflict first,
@@ -3467,6 +3472,8 @@ l2:
 	else
 		need_toast = (HeapTupleHasExternal(&oldtup) ||
 					  HeapTupleHasExternal(newtup) ||
+					  RelationGetDescr(relation)->tdflags & TD_ATTR_CUSTOM_COMPRESSED ||
+					  HeapTupleHasCustomCompressed(newtup) ||
 					  newtup->t_len > TOAST_TUPLE_THRESHOLD);
 
 	pagefree = PageGetHeapFreeSpace(page);
diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index 6082ee9a38..176416a943 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -1190,6 +1190,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 						(char *) new_data + new_header_len,
 						new_data_len,
 						&(new_data->t_infomask),
+						&(new_data->t_infomask2),
 						has_nulls ? new_data->t_bits : NULL);
 	}
 	else
@@ -1411,6 +1412,7 @@ toast_flatten_tuple_to_datum(HeapTupleHeader tup,
 					(char *) new_data + new_header_len,
 					new_data_len,
 					&(new_data->t_infomask),
+					&(new_data->t_infomask2),
 					has_nulls ? new_data->t_bits : NULL);
 
 	/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 218a6e01cb..acf00191b8 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2662,7 +2662,7 @@ CopyFrom(CopyState cstate)
 	values = (Datum *) palloc(tupDesc->natts * sizeof(Datum));
 	nulls = (bool *) palloc(tupDesc->natts * sizeof(bool));
 
-	bistate = GetBulkInsertState();
+	bistate = GetBulkInsertState(NULL);
 	econtext = GetPerTupleExprContext(estate);
 
 	/* Set up callback to identify error line number */
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 36e3d44aad..8a52a53472 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -559,7 +559,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	 */
 	myState->hi_options = HEAP_INSERT_SKIP_FSM |
 		(XLogIsNeeded() ? 0 : HEAP_INSERT_SKIP_WAL);
-	myState->bistate = GetBulkInsertState();
+	myState->bistate = GetBulkInsertState(NULL);
 
 	/* Not using WAL requires smgr_targblock be initially invalid */
 	Assert(RelationGetTargetBlock(intoRelationDesc) == InvalidBlockNumber);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 5a47be4b33..982a287e8c 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -464,7 +464,7 @@ transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	myState->hi_options = HEAP_INSERT_SKIP_FSM | HEAP_INSERT_FROZEN;
 	if (!XLogIsNeeded())
 		myState->hi_options |= HEAP_INSERT_SKIP_WAL;
-	myState->bistate = GetBulkInsertState();
+	myState->bistate = GetBulkInsertState(NULL);
 
 	/* Not using WAL requires smgr_targblock be initially invalid */
 	Assert(RelationGetTargetBlock(transientrel) == InvalidBlockNumber);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 90361f2a59..185c9650c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -165,6 +165,9 @@ typedef struct AlteredTableInfo
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
 	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
+	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	HTAB	   *preservedAmInfo;	/* Hash table for preserved compression
+									 * methods */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -4727,7 +4730,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	if (newrel)
 	{
 		mycid = GetCurrentCommandId(true);
-		bistate = GetBulkInsertState();
+		bistate = GetBulkInsertState(tab->preservedAmInfo);
 
 		hi_options = HEAP_INSERT_SKIP_FSM;
 		if (!XLogIsNeeded())
@@ -5016,6 +5019,24 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 
 	FreeExecutorState(estate);
 
+	/* Remove old compression options */
+	if (tab->rewrite & AT_REWRITE_ALTER_COMPRESSION)
+	{
+		AttrCmPreservedInfo *pinfo;
+		HASH_SEQ_STATUS status;
+
+		Assert(tab->preservedAmInfo);
+		hash_seq_init(&status, tab->preservedAmInfo);
+		while ((pinfo = (AttrCmPreservedInfo *) hash_seq_search(&status)) != NULL)
+		{
+			CleanupAttributeCompression(tab->relid, pinfo->attnum,
+										pinfo->preserved_amoids);
+			list_free(pinfo->preserved_amoids);
+		}
+
+		hash_destroy(tab->preservedAmInfo);
+	}
+
 	table_close(oldrel, NoLock);
 	if (newrel)
 	{
@@ -9505,6 +9526,45 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint,
 	CommandCounterIncrement();
 }
 
+/*
+ * Initialize hash table used to keep rewrite rules for
+ * compression changes in ALTER commands.
+ */
+static void
+setupCompressionRewriteRules(AlteredTableInfo *tab, const char *column,
+							 AttrNumber attnum, List *preserved_amoids)
+{
+	bool		found;
+	AttrCmPreservedInfo *pinfo;
+
+	Assert(!IsBinaryUpgrade);
+	tab->rewrite |= AT_REWRITE_ALTER_COMPRESSION;
+
+	/* initialize hash for oids */
+	if (tab->preservedAmInfo == NULL)
+	{
+		HASHCTL		ctl;
+
+		MemSet(&ctl, 0, sizeof(ctl));
+		ctl.keysize = sizeof(AttrNumber);
+		ctl.entrysize = sizeof(AttrCmPreservedInfo);
+		tab->preservedAmInfo =
+			hash_create("preserved access methods cache", 10, &ctl,
+						HASH_ELEM | HASH_BLOBS);
+	}
+	pinfo = (AttrCmPreservedInfo *) hash_search(tab->preservedAmInfo,
+												&attnum, HASH_ENTER, &found);
+
+	if (found)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot alter compression of column \"%s\" twice", column),
+				 errhint("Remove one of statements from the command.")));
+
+	pinfo->attnum = attnum;
+	pinfo->preserved_amoids = preserved_amoids;
+}
+
 /*
  * ALTER TABLE DROP CONSTRAINT
  *
@@ -10496,7 +10556,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 		{
 			/* Set up rewrite of table */
 			attTup->attcompression = InvalidOid;
-			/* TODO: call the rewrite function here */
+			setupCompressionRewriteRules(tab, colName, attOldTup->attnum, NIL);
 		}
 		else if (OidIsValid(attTup->attcompression))
 		{
@@ -10504,7 +10564,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 			ColumnCompression *compression = MakeColumnCompression(attTup->attcompression);
 
 			if (!IsBuiltinCompression(attTup->attcompression))
-				/* TODO: call the rewrite function here */;
+				setupCompressionRewriteRules(tab, colName, attOldTup->attnum, NIL);
 
 			attTup->attcompression = CreateAttributeCompression(attTup, compression, NULL, NULL);
 		}
@@ -13620,7 +13680,8 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	 * toast_insert_or_update
 	 */
 	if (need_rewrite)
-		/* TODO: set up rewrite rules here */;
+		setupCompressionRewriteRules(tab, column, atttableform->attnum,
+									 preserved_amoids);
 
 	atttableform->attcompression = acoid;
 	CatalogTupleUpdate(attrel, &atttuple->t_self, atttuple);
diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c
index 9971abd71f..71fffc53b8 100644
--- a/src/backend/utils/adt/expandedrecord.c
+++ b/src/backend/utils/adt/expandedrecord.c
@@ -808,6 +808,7 @@ ER_flatten_into(ExpandedObjectHeader *eohptr,
 					(char *) tuphdr + erh->hoff,
 					erh->data_len,
 					&tuphdr->t_infomask,
+					&tuphdr->t_infomask2,
 					(erh->hasnull ? tuphdr->t_bits : NULL));
 }
 
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index eba77491fd..6fa67242ef 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -569,6 +569,10 @@ RelationBuildTupleDesc(Relation relation)
 			ndef++;
 		}
 
+		/* mark tupledesc as it contains attributes with custom compression */
+		if (attp->attcompression)
+			relation->rd_att->tdflags |= TD_ATTR_CUSTOM_COMPRESSED;
+
 		/* Likewise for a missing value */
 		if (attp->atthasmissing)
 		{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 1b6607fe90..aa54b52476 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -25,6 +25,7 @@
 #include "storage/lockdefs.h"
 #include "utils/relcache.h"
 #include "utils/snapshot.h"
+#include "utils/hsearch.h"
 
 
 /* "options" flag bits for heap_insert */
@@ -162,7 +163,7 @@ extern void heap_get_latest_tid(Relation relation, Snapshot snapshot,
 					ItemPointer tid);
 extern void setLastTid(const ItemPointer tid);
 
-extern BulkInsertState GetBulkInsertState(void);
+extern BulkInsertState GetBulkInsertState(HTAB *);
 extern void FreeBulkInsertState(BulkInsertState);
 extern void ReleaseBulkInsertStatePin(BulkInsertState bistate);
 
diff --git a/src/include/access/hio.h b/src/include/access/hio.h
index cec087cb1a..84c736cea2 100644
--- a/src/include/access/hio.h
+++ b/src/include/access/hio.h
@@ -31,6 +31,8 @@ typedef struct BulkInsertStateData
 {
 	BufferAccessStrategy strategy;	/* our BULKWRITE strategy object */
 	Buffer		current_buf;	/* current insertion target page */
+	HTAB	   *preserved_am_info;	/* hash table with preserved compression
+									 * methods for attributes */
 }			BulkInsertStateData;
 
 
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 6d51f9062b..1ab905a131 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -274,7 +274,9 @@ struct HeapTupleHeaderData
  * information stored in t_infomask2:
  */
 #define HEAP_NATTS_MASK			0x07FF	/* 11 bits for number of attributes */
-/* bits 0x1800 are available */
+/* bit 0x800 is available */
+#define HEAP_HASCUSTOMCOMPRESSED 0x1000 /* tuple contains custom compressed
+										 * varlena(s) */
 #define HEAP_KEYS_UPDATED		0x2000	/* tuple was updated and key cols
 										 * modified, or tuple deleted */
 #define HEAP_HOT_UPDATED		0x4000	/* tuple was HOT-updated */
@@ -673,6 +675,9 @@ struct MinimalTupleData
 #define HeapTupleHasExternal(tuple) \
 		(((tuple)->t_data->t_infomask & HEAP_HASEXTERNAL) != 0)
 
+#define HeapTupleHasCustomCompressed(tuple) \
+		(((tuple)->t_data->t_infomask2 & HEAP_HASCUSTOMCOMPRESSED) != 0)
+
 #define HeapTupleIsHotUpdated(tuple) \
 		HeapTupleHeaderIsHotUpdated((tuple)->t_data)
 
@@ -779,7 +784,7 @@ extern Size heap_compute_data_size(TupleDesc tupleDesc,
 extern void heap_fill_tuple(TupleDesc tupleDesc,
 				Datum *values, bool *isnull,
 				char *data, Size data_size,
-				uint16 *infomask, bits8 *bit);
+				uint16 *infomask, uint16 *infomask2, bits8 *bit);
 extern bool heap_attisnull(HeapTuple tup, int attnum, TupleDesc tupleDesc);
 extern Datum nocachegetattr(HeapTuple tup, int attnum,
 			   TupleDesc att);
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index 66d1b2fc40..c878a0636a 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -14,7 +14,9 @@
 #ifndef TUPDESC_H
 #define TUPDESC_H
 
+#include "postgres.h"
 #include "access/attnum.h"
+#include "access/cmapi.h"
 #include "catalog/pg_attribute.h"
 #include "nodes/pg_list.h"
 
@@ -44,6 +46,10 @@ typedef struct TupleConstr
 	bool		has_not_null;
 } TupleConstr;
 
+/* tupledesc flags */
+#define TD_ATTR_CUSTOM_COMPRESSED	0x01	/* is TupleDesc contain attributes
+											 * with custom compression? */
+
 /*
  * This struct is passed around within the backend to describe the structure
  * of tuples.  For tuples coming from on-disk relations, the information is
@@ -80,6 +86,7 @@ typedef struct TupleDescData
 	int			natts;			/* number of attributes in the tuple */
 	Oid			tdtypeid;		/* composite type ID for tuple type */
 	int32		tdtypmod;		/* typmod for tuple type */
+	char		tdflags;		/* tuple additional flags */
 	int			tdrefcount;		/* reference count, or -1 if not counting */
 	TupleConstr *constr;		/* constraints, or NULL if none */
 	/* attrs[N] is the description of Attribute Number N+1 */
diff --git a/src/include/access/tuptoaster.h b/src/include/access/tuptoaster.h
index fad14bad85..aaf3b155dc 100644
--- a/src/include/access/tuptoaster.h
+++ b/src/include/access/tuptoaster.h
@@ -13,7 +13,6 @@
 #ifndef TUPTOASTER_H
 #define TUPTOASTER_H
 
-#include "access/cmapi.h"
 #include "access/htup_details.h"
 #include "storage/lockdefs.h"
 #include "utils/relcache.h"
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 0b8c7cca21..9252ad3d3a 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -31,6 +31,7 @@ typedef struct EventTriggerData
 #define AT_REWRITE_ALTER_PERSISTENCE	0x01
 #define AT_REWRITE_DEFAULT_VAL			0x02
 #define AT_REWRITE_COLUMN_REWRITE		0x04
+#define AT_REWRITE_ALTER_COMPRESSION	0x08
 
 /*
  * EventTriggerData is the node type that is passed as fmgr "context" info
-- 
2.21.0


--MP_//XvyHCr_hJMvh/tp2R3=uJa
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0004-Add-pglz-compression-method-v22.patch



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
@ 2022-02-24 08:27 Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-02-24 08:27 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; pgsql-hackers

At Thu, 24 Feb 2022 16:26:42 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in 
> So, actually WAL did not ended in an incomplete record.  I think
> FinishWalRecover is the last place to do that. (But it could be
> earlier.)

After some investigation, I finally concluded that we should reset
abortedRecPtr and missingContrecPtr at processing
XLOG_OVERWRITE_CONTRECORD.


--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1953,6 +1953,11 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
                                                LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
                                                timestamptz_to_str(xlrec.overwrite_time))));
 
+               /* We have safely skipped the aborted record */
+               abortedRecPtr = InvalidXLogRecPtr;
+               missingContrecPtr = InvalidXLogRecPtr;
+
                /* Verifying the record should only happen once */
                record->overwrittenRecPtr = InvalidXLogRecPtr;
        }

The last check in the test against "resetting aborted record" is no
longer useful since it is already checked by
026_verwrite_contrecord.pl.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-02-25 21:28 ` Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-02-25 21:28 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers

>    After some investigation, I finally concluded that we should reset
>    abortedRecPtr and missingContrecPtr at processing
>    XLOG_OVERWRITE_CONTRECORD.


 >   --- a/src/backend/access/transam/xlogrecovery.c
 >   +++ b/src/backend/access/transam/xlogrecovery.c
 >   @@ -1953,6 +1953,11 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
                                                    LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
                                                    timestamptz_to_str(xlrec.overwrite_time))));

 >   +               /* We have safely skipped the aborted record */
 >   +               abortedRecPtr = InvalidXLogRecPtr;
 >   +               missingContrecPtr = InvalidXLogRecPtr;
 >   +
 >                   /* Verifying the record should only happen once */
 >                   record->overwrittenRecPtr = InvalidXLogRecPtr;
 >           }

>    The last check in the test against "resetting aborted record" is no
>    longer useful since it is already checked by
>    026_verwrite_contrecord.pl.

>    regards.

+1 for this. Resetting abortedRecPtr and missingContrecPtr after the broken record is skipped in is cleaner. I also think it would make sense to move the " successfully skipped missing contrecord" to after we invalidate the aborted and missing record pointers, like below.

+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1948,15 +1948,15 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
                                 LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
                                 LSN_FORMAT_ARGS(record->overwrittenRecPtr));

+               /* We have safely skipped the aborted record */
+               abortedRecPtr = InvalidXLogRecPtr;
+               missingContrecPtr = InvalidXLogRecPtr;
+
                ereport(LOG,
                                (errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
                                                LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
                                                timestamptz_to_str(xlrec.overwrite_time))));

Also, instead of a new test, the current 026_overwrite_contrecord.pl test can include a promotion test at the end.

Attaching a new patch for review.

 --
Sami Imseih
Amazon Web Services



Attachments:

  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot.patch (2.4K, ../../[email protected]/2-v4-0001-Fix-missing-continuation-record-after-standby-promot.patch)
  download | inline diff:
From a43c4dc01c27a07798c935cbc15260d1a668d3b9 Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Fri, 25 Feb 2022 21:23:34 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 src/backend/access/transam/xlogrecovery.c       | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index f9f212680b..bbb2d7fef5 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1948,6 +1948,10 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
 				 LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
 				 LSN_FORMAT_ARGS(record->overwrittenRecPtr));
 
+		/* We have safely skipped the aborted record */
+		abortedRecPtr = InvalidXLogRecPtr;
+		missingContrecPtr = InvalidXLogRecPtr;
+
 		ereport(LOG,
 				(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 						LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 0fd907f152..fe175f5e90 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -13,7 +13,7 @@ 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgreSQL::Test::Cluster->new('primary');
 $node->init(allows_streaming => 1);
@@ -100,6 +100,9 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$node_standby->safe_psql('postgres', "select pg_promote()");
+
 $node->stop;
 $node_standby->stop;
 
-- 
2.32.0



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-03-07 20:34   ` Imseih (AWS), Sami <[email protected]>
  2022-03-23 17:24     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion [email protected] <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-03-07 20:34 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers

I have gone ahead and backpatched this all the way to 10 as well.

 --
 Sami Imseih
 Amazon Web Services




Attachments:

  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot-v10.patch (2.5K, ../../[email protected]/2-v4-0001-Fix-missing-continuation-record-after-standby-promot-v10.patch)
  download | inline diff:
From ae10aaed56ba309bcb8a32d74a7f5fc9ea466c5c Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Mon, 7 Mar 2022 19:43:50 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]

Backpatch to 10
---
 src/backend/access/transam/xlog.c               | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c64febdb53..76fe62af33 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10260,6 +10260,10 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state
 			 (uint32) (state->overwrittenRecPtr >> 32),
 			 (uint32) state->overwrittenRecPtr);
 
+	/* We have safely skipped the aborted record */
+	abortedRecPtr = InvalidXLogRecPtr;
+	missingContrecPtr = InvalidXLogRecPtr;
+
 	ereport(LOG,
 			(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 					(uint32) (xlrec->overwritten_lsn >> 32),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 57b2a6b7fb..ef962b1271 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -15,7 +15,7 @@ plan tests => 3;
 # 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgresNode->get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -105,5 +105,11 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$ENV{PGDATA} = $node_standby->data_dir;
+$ENV{PGPORT} = $node_standby->port;
+$ENV{PGGHOST} = $node_standby->host;
+system "pg_ctl promote";
+
 $node->stop;
 $node_standby->stop;
-- 
2.32.0



  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot-v11.patch (2.5K, ../../[email protected]/3-v4-0001-Fix-missing-continuation-record-after-standby-promot-v11.patch)
  download | inline diff:
From 6fab9a83c7a6fce4f2fdd748697f23921a5df2ed Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Mon, 7 Mar 2022 19:49:32 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]

Backpatch to 10
---
 src/backend/access/transam/xlog.c               | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 12992f5f3d..5afb3a5f1d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10296,6 +10296,10 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state
 			 (uint32) (state->overwrittenRecPtr >> 32),
 			 (uint32) state->overwrittenRecPtr);
 
+	/* We have safely skipped the aborted record */
+	abortedRecPtr = InvalidXLogRecPtr;
+	missingContrecPtr = InvalidXLogRecPtr;
+
 	ereport(LOG,
 			(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 					(uint32) (xlrec->overwritten_lsn >> 32),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 867b75937e..f51547e149 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -15,7 +15,7 @@ plan tests => 3;
 # 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgresNode->get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -102,5 +102,11 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$ENV{PGDATA} = $node_standby->data_dir;
+$ENV{PGPORT} = $node_standby->port;
+$ENV{PGGHOST} = $node_standby->host;
+system "pg_ctl promote";
+
 $node->stop;
 $node_standby->stop;
-- 
2.32.0



  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot-v12.patch (2.4K, ../../[email protected]/4-v4-0001-Fix-missing-continuation-record-after-standby-promot-v12.patch)
  download | inline diff:
From 4daed94b2c32c1eb11cf37e87d11ea2c5ae803cd Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Fri, 4 Mar 2022 19:51:15 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]

Backpatch to 10
---
 src/backend/access/transam/xlog.c               | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 885558f291..8c7bb1dc6f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10148,6 +10148,10 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state
 			 (uint32) (state->overwrittenRecPtr >> 32),
 			 (uint32) state->overwrittenRecPtr);
 
+	/* We have safely skipped the aborted record */
+	abortedRecPtr = InvalidXLogRecPtr;
+	missingContrecPtr = InvalidXLogRecPtr;
+
 	ereport(LOG,
 			(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 					(uint32) (xlrec->overwritten_lsn >> 32),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 867b75937e..9115b5ffc9 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -15,7 +15,7 @@ plan tests => 3;
 # 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgresNode->get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -102,5 +102,8 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$node_standby->safe_psql('postgres', "select pg_promote()");
+
 $node->stop;
 $node_standby->stop;
-- 
2.32.0



  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot-v13.patch (2.4K, ../../[email protected]/5-v4-0001-Fix-missing-continuation-record-after-standby-promot-v13.patch)
  download | inline diff:
From 79fb2465daf73bf9f710e55860f68df755d614b8 Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Fri, 4 Mar 2022 23:57:02 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]

Backpatch to 10
---
 src/backend/access/transam/xlog.c               | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3d76fad128..a472e26328 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10382,6 +10382,10 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state
 			 (uint32) (state->overwrittenRecPtr >> 32),
 			 (uint32) state->overwrittenRecPtr);
 
+	/* We have safely skipped the aborted record */
+	abortedRecPtr = InvalidXLogRecPtr;
+	missingContrecPtr = InvalidXLogRecPtr;
+
 	ereport(LOG,
 			(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 					(uint32) (xlrec->overwritten_lsn >> 32),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index cb7f5b68c6..2577afc98b 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -15,7 +15,7 @@ plan tests => 3;
 # 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgresNode->get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -102,5 +102,8 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$node_standby->safe_psql('postgres', "select pg_promote()");
+
 $node->stop;
 $node_standby->stop;
-- 
2.32.0



  [application/octet-stream] v4-0001-Fix-missing-continuation-record-after-standby-promot-v14.patch (2.4K, ../../[email protected]/6-v4-0001-Fix-missing-continuation-record-after-standby-promot-v14.patch)
  download | inline diff:
From 8332d475825f05f5c9f7f48537d4c5cbbc6f29c4 Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Fri, 4 Mar 2022 19:10:18 +0000
Subject: [PATCH 1/1] Fix "missing continuation record" after standby promotion

Invalidate abortedRecPtr and missingContrecPtr after a missing
continuation record is skipped on a standby. This fixes a PANIC
caused when a recently promoted standby attempts to write an
OVERWRITE_RECORD with an LSN of the previously read aborted record.

Author: Sami Imseih
Reviewed-by: Kyotaro Horiguchi <[email protected]>, Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]

Backpatch to 10
---
 src/backend/access/transam/xlog.c               | 4 ++++
 src/test/recovery/t/026_overwrite_contrecord.pl | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6208e123e5..b241f33f7e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10591,6 +10591,10 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state
 			 LSN_FORMAT_ARGS(xlrec->overwritten_lsn),
 			 LSN_FORMAT_ARGS(state->overwrittenRecPtr));
 
+	/* We have safely skipped the aborted record */
+	abortedRecPtr = InvalidXLogRecPtr;
+	missingContrecPtr = InvalidXLogRecPtr;
+
 	ereport(LOG,
 			(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 					LSN_FORMAT_ARGS(xlrec->overwritten_lsn),
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index cb7f5b68c6..2577afc98b 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -15,7 +15,7 @@ plan tests => 3;
 # 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 replays the "overwrite contrecord" from that new
-# file.
+# file and the standby promotes successfully.
 
 my $node = PostgresNode->get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -102,5 +102,8 @@ like(
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
 
+# Verify promotion is successful
+$node_standby->safe_psql('postgres', "select pg_promote()");
+
 $node->stop;
 $node_standby->stop;
-- 
2.32.0



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-03-23 17:24     ` [email protected] <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: [email protected] @ 2022-03-23 17:24 UTC (permalink / raw)
  To: Imseih (AWS), Sami <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers

On 2022-Mar-07, Imseih (AWS), Sami wrote:

> I have gone ahead and backpatched this all the way to 10 as well.

Thanks!  I pushed this now.  I edited the test though: I don't
understand why you went to the trouble of setting stuff in order to call
'pg_ctl promote' (in different ways for older branches), when
$node_standby->promote does the same and is simpler to call.  So I
changed the tests to do that.  (I did verify that without the code fix,
the PANIC indeed is thrown.)

Thank again,

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-05-26 19:57     ` Imseih (AWS), Sami <[email protected]>
  2022-05-27 01:59       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  1 sibling, 2 replies; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-05-26 19:57 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers

We see another occurrence of this bug with the last patch applied in 13.7.

After a promotion we observe the following in the logs:

2022-05-25 00:35:38 UTC::@:[371]:PANIC: xlog flush request 10/B1FA3D88 is not satisfied --- flushed only to 7/A8000060
2022-05-25 00:35:38 UTC:172.31.26.238(38610):administrator@postgres:[23433]:ERROR: current transaction is aborted, commands ignored until end of transaction block

However, The logs do not show "LOG: successfully skipped missing contrecord",
therefore we know that  VerifyOverwriteContrecord 
is not being called to invalidate the missingContrecPtr.

VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state)
{
   if (xlrec->overwritten_lsn != state->overwrittenRecPtr)
       elog(FATAL, "mismatching overwritten LSN %X/%X -> %X/%X",
            (uint32) (xlrec->overwritten_lsn >> 32),
            (uint32) xlrec->overwritten_lsn,
            (uint32) (state->overwrittenRecPtr >> 32),
            (uint32) state->overwrittenRecPtr);

   /* We have safely skipped the aborted record */
   abortedRecPtr = InvalidXLogRecPtr;
   missingContrecPtr = InvalidXLogRecPtr;

   ereport(LOG,
           (errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
                   (uint32) (xlrec->overwritten_lsn >> 32),
                   (uint32) xlrec->overwritten_lsn,
                   timestamptz_to_str(xlrec->overwrite_time))));

We think it's because VerifyOverwriteContrecord was not 
called which is why we see this behavior. 

Are there are  other places where missingContrecPtr 
should be invalidated, such as after a successful promotion?

--
Sami Imseih
Amazon Web Services





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-05-27 01:59       ` Kyotaro Horiguchi <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-05-27 01:59 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; pgsql-hackers

At Thu, 26 May 2022 19:57:41 +0000, "Imseih (AWS), Sami" <[email protected]> wrote in 
> We see another occurrence of this bug with the last patch applied in 13.7.
> 
> After a promotion we observe the following in the logs:
...
> We think it's because VerifyOverwriteContrecord was not 
> called which is why we see this behavior. 
> 
> Are there are  other places where missingContrecPtr 
> should be invalidated, such as after a successful promotion?

The only cause known to me for EndOfLog being moved to such location
is missingContrecPtr. But if the next record is not
XLOG_OVERWRITE_CONTRECORD recovery should have stopped there. And if
XLOG_OVERWRITE_CONTRECORD is there, VerifyOverwriteContrecord should
have been called..

Could you inspect WAL files of the environment and see if the first
record of the '7/A8'th segment OVERWRITE_CONTRECORD?  I don't say that
makes some progress on this, but could be the first step.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-05-27 02:01       ` Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-05-27 02:01 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers

After further research, we found the following.

Testing on 13.6 with the attached patch we see
that the missingContrecPtr is being incorrectly
set on the standby and the promote in the tap
test fails.

Per the comments in xlog.c, the
missingContrecPtr should not be set when
in standby mode.

            /*
             * When not in standby mode we find that WAL ends in an incomplete
             * record, keep track of that record.  After recovery is done,
             * we'll write a record to indicate downstream WAL readers that
             * that portion is to be ignored.
             */
            if (!StandbyMode &&
                !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
            {
                abortedRecPtr = xlogreader->abortedRecPtr;
                missingContrecPtr = xlogreader->missingContrecPtr;
                elog(LOG, "missingContrecPtr == %ld", missingContrecPtr);
            }

If StandbyModeRequested is checked instead, which
checks for the presence of a standby signal file,
The missingContrecPtr is not set on the
standby and the test succeeds.

            if (!StandbyModeRequested &&
                !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
            {
                abortedRecPtr = xlogreader->abortedRecPtr;
                missingContrecPtr = xlogreader->missingContrecPtr;
                elog(LOG, "missingContrecPtr == %ld", missingContrecPtr);
            }

If this is a bug as it appears, it appears the original patch
to resolve this issue is not needed and the ideal fix
Is to ensure that a standby does not set
missingContrecPtr.

Would like to see what others think.

Thanks

--
Sami Imseih
Amazon Web Services




Attachments:

  [application/octet-stream] 0001-Testing_patch[2][2].patch (1.5K, ../../[email protected]/2-0001-Testing_patch%5B2%5D%5B2%5D.patch)
  download | inline diff:
From 736308047747c7b1e5b78dacf95a0b8459d2b7a1 Mon Sep 17 00:00:00 2001
From: "Imseih (AWS)" <[email protected]>
Date: Thu, 26 May 2022 19:41:22 -0500
Subject: [PATCH 1/1] Testing_patch

---
 src/backend/access/transam/xlog.c               | 2 ++
 src/test/recovery/t/026_overwrite_contrecord.pl | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3d76fad..a1b78f3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4381,6 +4381,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			{
 				abortedRecPtr = xlogreader->abortedRecPtr;
 				missingContrecPtr = xlogreader->missingContrecPtr;
+				elog(LOG, "missingContrecPtr == %ld", missingContrecPtr);
 			}
 
 			if (readFile >= 0)
@@ -4398,6 +4399,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			if (errormsg)
 				ereport(emode_for_corrupt_record(emode, EndRecPtr),
 						(errmsg_internal("%s", errormsg) /* already translated */ ));
+
 		}
 
 		/*
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index cb7f5b6..a3000f1 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -101,6 +101,6 @@ like(
 	$log,
 	qr[successfully skipped missing contrecord at],
 	"found log line in standby");
-
+$node_standby->promote;
 $node->stop;
 $node_standby->stop;
-- 
2.32.1 (Apple Git-133)



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-05-27 06:37         ` Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-05-27 06:37 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; pgsql-hackers

At Fri, 27 May 2022 02:01:27 +0000, "Imseih (AWS), Sami" <[email protected]> wrote in 
> After further research, we found the following.
> 
> Testing on 13.6 with the attached patch we see
> that the missingContrecPtr is being incorrectly
> set on the standby and the promote in the tap
> test fails.
> 
> Per the comments in xlog.c, the
> missingContrecPtr should not be set when
> in standby mode.
> 
>             /*
>              * When not in standby mode we find that WAL ends in an incomplete
>              * record, keep track of that record.  After recovery is done,
>              * we'll write a record to indicate downstream WAL readers that
>              * that portion is to be ignored.
>              */
>             if (!StandbyMode &&
>                 !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
>             {
>                 abortedRecPtr = xlogreader->abortedRecPtr;
>                 missingContrecPtr = xlogreader->missingContrecPtr;
>                 elog(LOG, "missingContrecPtr == %ld", missingContrecPtr);
>             }
> 
> If StandbyModeRequested is checked instead, which
> checks for the presence of a standby signal file,
> The missingContrecPtr is not set on the
> standby and the test succeeds.
> 
>             if (!StandbyModeRequested &&
>                 !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
>             {
>                 abortedRecPtr = xlogreader->abortedRecPtr;
>                 missingContrecPtr = xlogreader->missingContrecPtr;
>                 elog(LOG, "missingContrecPtr == %ld", missingContrecPtr);
>             }
> 
> If this is a bug as it appears, it appears the original patch
> to resolve this issue is not needed and the ideal fix
> Is to ensure that a standby does not set
> missingContrecPtr.
> 
> Would like to see what others think.

Due to lack of information, I don't have a clear idea of what is
happening here. If that change "fixed" the "issue", that seems to mean
that crash recovery passed an immature contrecord (@6/A8000060) but
recovery did not stop at the time then continues until 10/B1FA3D88.  A
possibility is, crash recovery ended at the immature contrecord then
the server moved to archive recovery mode and was able to continue
from the same (aborted) LSN on the archived WAL. This means 6/A8 in
pg_wal had been somehow removed after archived, or the segment 6/A7
(not A8) in both pg_xlog and archive are in different histories, which
seems to me what we wanted to prevent by the
XLOG_OVERWRITE_CONTRECORD.  Didn't you use the same archive content
for repeated recovery testing?

And from the fact that entering crash recovery means the cluster
didn't have an idea of how far it should recover until consistency,
that is contained in backup label control file.  That could happen
when a crashed primary is as-is reused as the new standby of the new
primary.

So.. I'd like to hear exactly what you did as the testing.

When standby mode is requested, if crash recovery fails with immature
contrecord, I think we shouldn't continue recovery. But I'm not sure
we need to explictly reject that case.  Further study is needed..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-05-27 19:01           ` Imseih (AWS), Sami <[email protected]>
  2022-06-02 18:28             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Hsu, John <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-05-27 19:01 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers

>    So.. I'd like to hear exactly what you did as the testing.

>    When standby mode is requested, if crash recovery fails with immature
>    contrecord, I think we shouldn't continue recovery. But I'm not sure
>    we need to explictly reject that case.  Further study is needed..


Here is more details about my findings and testing.

Even with commit 9d92582abf918215d27659d45a4c9e78bda50aff 
we still see the issue with post promotion checkpoint
resulting in "request to flush past end of generated WAL;"

i.e.

2022-05-25 00:35:38 UTC::@:[371]:LOG:  checkpoint starting: immediate force wait wal time
2022-05-25 00:35:38 UTC::@:[371]:LOG:  request to flush past end of generated WAL; request 10/B1FA3D88, currpos 7/A8000060
2022-05-25 00:35:38 UTC::@:[371]:PANIC:  xlog flush request 10/B1FA3D88 is not satisfied --- flushed only to 7/A8000060
2022-05-25 00:35:38 UTC:172.31.26.238(38610):administrator@postgres:[23433]:ERROR:  current transaction is aborted, commands ignored until end of transaction block

The intent of commit 9d92582abf918215d27659d45a4c9e78bda50aff 
was to make sure the standby skips the overwrite contrecord.

However, we still see "missingContrecPtr" is being set 
on the standby before promotion and after the instance is 
promoted, the missingContrecPtr is written to WAL 
and the subsequent flush throws a "PANIC:  xlog flush request"

To Reproduce using TAP tests; 

1) apply the attached patch 0001-Patch_to_repro.patch to the head branch.
   This patch adds logging when an OverWriteContRecord
   is created and comments out the invalidation code
   added in commit  9d92582abf918215d27659d45a4c9e78bda50aff
   
2) Run a tap test under "test/recovery"
   make check PROVE_TESTS='t/026_overwrite_contrecord.pl'

3) What is observed in the logfiles for both the standby
   and primary instance in the tap test is 
   that an overwrite contrecord is created on the primary, 
   which is correct, but also on the standby after promotion, 
   which is incorrect. This is incorrect as the contrecord

simseih@88665a22795f recovery % cat tmp_check/log/*prim* | grep 'creating\|promo'
2022-05-27 13:17:50.843 CDT [98429] LOG:  creating overwrite contrecord at 0/2000058 for aborted_lsn 0/1FFD000

simseih@88665a22795f recovery % cat tmp_check/log/*stan* | grep 'creating\|promo'
2022-05-27 13:17:51.361 CDT [98421] LOG:  received promote request
2022-05-27 13:17:51.394 CDT [98421] LOG:  creating overwrite contrecord at 0/2000058 for aborted_lsn 0/1FFD000
simseih@88665a22795f recovery %

What we found:

1. missingContrecPtr is set when 
   StandbyMode is false, therefore
   only a writer should set this value
   and a record is then sent downstream.

   But a standby going through crash 
   recovery will always have StandbyMode = false,
   causing the missingContrecPtr to be incorrectly
   set.

2. If StandbyModeRequested is checked instead,
     we ensure that a standby will not set a 
     missingContrecPtr.

3. After applying the patch below, the tap test succeeded

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 5ee90b6..a727aaf 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2977,7 +2977,7 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
                         * we'll write a record to indicate to downstream WAL readers that
                         * that portion is to be ignored.
                         */
-                       if (!StandbyMode &&
+                       if (!StandbyModeRequested &&
                                !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
                        {
                                abortedRecPtr = xlogreader->abortedRecPtr;

So, it might be that the best fix is not the commit in 9d92582abf918215d27659d45a4c9e78bda50aff
but to check StandbyModeRequested = false before setting 
missingContrecPtr.

Thank you
---
Sami Imseih
Amazon Web Services



Attachments:

  [application/octet-stream] 0001-Patch_to_repro.patch (1.6K, ../../[email protected]/2-0001-Patch_to_repro.patch)
  download | inline diff:
From 657570f32c8151dcd74eeb4810d97b090c3ac889 Mon Sep 17 00:00:00 2001
From: "Imseih (AWS)" <[email protected]>
Date: Fri, 27 May 2022 13:28:50 -0500
Subject: [PATCH 1/1] Patch_to_repro

---
 src/backend/access/transam/xlog.c         | 2 ++
 src/backend/access/transam/xlogrecovery.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 71136b1..dc301ed 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6813,6 +6813,8 @@ CreateOverwriteContrecordRecord(XLogRecPtr aborted_lsn, XLogRecPtr pagePtr,
 		elog(ERROR, "OVERWRITE_CONTRECORD was inserted to unexpected position %X/%X",
 			 LSN_FORMAT_ARGS(ProcLastRecPtr));
 
+	elog(LOG, "creating overwrite contrecord at %X/%X for aborted_lsn %X/%X",
+				LSN_FORMAT_ARGS(recptr), LSN_FORMAT_ARGS(aborted_lsn));
 	XLogFlush(recptr);
 
 	END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626..5ee90b6 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1971,8 +1971,8 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
 				 LSN_FORMAT_ARGS(record->overwrittenRecPtr));
 
 		/* We have safely skipped the aborted record */
-		abortedRecPtr = InvalidXLogRecPtr;
-		missingContrecPtr = InvalidXLogRecPtr;
+		//abortedRecPtr = InvalidXLogRecPtr;
+		//missingContrecPtr = InvalidXLogRecPtr;
 
 		ereport(LOG,
 				(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
-- 
2.32.1 (Apple Git-133)



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-06-02 18:28             ` Hsu, John <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Hsu, John @ 2022-06-02 18:28 UTC (permalink / raw)
  To: Imseih (AWS), Sami <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers

Hi,

As an update we have a test suite running for the last few days with 
constant workload and failovers/promotions. With the proposed change 
from Sami we no longer see PANICs due to this.

The comment indicates that the abortedRecPtr and missingContRecPtr 
should only be set when the database is a writer since it's used to 
write a new record to downstream readers. !StandbyMode is a poor proxy 
for this as Sami mentioned since there's cases when a replica is going 
through crash recovery that would then set this record.

Thanks,

John H

On 5/27/22 12:01 PM, Imseih (AWS), Sami wrote:
>>     So.. I'd like to hear exactly what you did as the testing.
>>     When standby mode is requested, if crash recovery fails with immature
>>     contrecord, I think we shouldn't continue recovery. But I'm not sure
>>     we need to explictly reject that case.  Further study is needed..
>
> Here is more details about my findings and testing.
>
> Even with commit 9d92582abf918215d27659d45a4c9e78bda50aff
> we still see the issue with post promotion checkpoint
> resulting in "request to flush past end of generated WAL;"
>
> i.e.
>
> 2022-05-25 00:35:38 UTC::@:[371]:LOG:  checkpoint starting: immediate force wait wal time
> 2022-05-25 00:35:38 UTC::@:[371]:LOG:  request to flush past end of generated WAL; request 10/B1FA3D88, currpos 7/A8000060
> 2022-05-25 00:35:38 UTC::@:[371]:PANIC:  xlog flush request 10/B1FA3D88 is not satisfied --- flushed only to 7/A8000060
> 2022-05-25 00:35:38 UTC:172.31.26.238(38610):administrator@postgres:[23433]:ERROR:  current transaction is aborted, commands ignored until end of transaction block
>
> The intent of commit 9d92582abf918215d27659d45a4c9e78bda50aff
> was to make sure the standby skips the overwrite contrecord.
>
> However, we still see "missingContrecPtr" is being set
> on the standby before promotion and after the instance is
> promoted, the missingContrecPtr is written to WAL
> and the subsequent flush throws a "PANIC:  xlog flush request"
>
> To Reproduce using TAP tests;
>
> 1) apply the attached patch 0001-Patch_to_repro.patch to the head branch.
>     This patch adds logging when an OverWriteContRecord
>     is created and comments out the invalidation code
>     added in commit  9d92582abf918215d27659d45a4c9e78bda50aff
>     
> 2) Run a tap test under "test/recovery"
>     make check PROVE_TESTS='t/026_overwrite_contrecord.pl'
>
> 3) What is observed in the logfiles for both the standby
>     and primary instance in the tap test is
>     that an overwrite contrecord is created on the primary,
>     which is correct, but also on the standby after promotion,
>     which is incorrect. This is incorrect as the contrecord
>
> simseih@88665a22795f recovery % cat tmp_check/log/*prim* | grep 'creating\|promo'
> 2022-05-27 13:17:50.843 CDT [98429] LOG:  creating overwrite contrecord at 0/2000058 for aborted_lsn 0/1FFD000
>
> simseih@88665a22795f recovery % cat tmp_check/log/*stan* | grep 'creating\|promo'
> 2022-05-27 13:17:51.361 CDT [98421] LOG:  received promote request
> 2022-05-27 13:17:51.394 CDT [98421] LOG:  creating overwrite contrecord at 0/2000058 for aborted_lsn 0/1FFD000
> simseih@88665a22795f recovery %
>
> What we found:
>
> 1. missingContrecPtr is set when
>     StandbyMode is false, therefore
>     only a writer should set this value
>     and a record is then sent downstream.
>
>     But a standby going through crash
>     recovery will always have StandbyMode = false,
>     causing the missingContrecPtr to be incorrectly
>     set.
>
> 2. If StandbyModeRequested is checked instead,
>       we ensure that a standby will not set a
>       missingContrecPtr.
>
> 3. After applying the patch below, the tap test succeeded
>
> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
> index 5ee90b6..a727aaf 100644
> --- a/src/backend/access/transam/xlogrecovery.c
> +++ b/src/backend/access/transam/xlogrecovery.c
> @@ -2977,7 +2977,7 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
>                           * we'll write a record to indicate to downstream WAL readers that
>                           * that portion is to be ignored.
>                           */
> -                       if (!StandbyMode &&
> +                       if (!StandbyModeRequested &&
>                                  !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
>                          {
>                                  abortedRecPtr = xlogreader->abortedRecPtr;
>
> So, it might be that the best fix is not the commit in 9d92582abf918215d27659d45a4c9e78bda50aff
> but to check StandbyModeRequested = false before setting
> missingContrecPtr.
>
> Thank you
> ---
> Sami Imseih
> Amazon Web Services
>





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-06-20 07:13             ` Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Michael Paquier @ 2022-06-20 07:13 UTC (permalink / raw)
  To: Imseih (AWS), Sami <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; pgsql-hackers

On Fri, May 27, 2022 at 07:01:37PM +0000, Imseih (AWS), Sami wrote:
> What we found:
> 
> 1. missingContrecPtr is set when 
>    StandbyMode is false, therefore
>    only a writer should set this value
>    and a record is then sent downstream.
> 
>    But a standby going through crash 
>    recovery will always have StandbyMode = false,
>    causing the missingContrecPtr to be incorrectly
>    set.

That stands as true as far as I know, StandbyMode would be switched
only once we get out of crash recovery, and only if archive recovery
completes when there is a restore_command.

> 2. If StandbyModeRequested is checked instead,
>      we ensure that a standby will not set a 
>      missingContrecPtr.
> 
> 3. After applying the patch below, the tap test succeeded

Hmm.  I have not looked at that in depth, but if the intention is to
check that the database is able to write WAL, looking at
XLogCtl->SharedRecoveryState would be the way to go because that's the
flip switching between crash recovery, archive recovery and the end of
recovery (when WAL can be safely written).

The check in xlogrecovery_redo() still looks like a good thing to have
anyway, because we know that we can safely skip the contrecord.  Now,
for any patch produced, could the existing TAP test be extended so as
we are able to get a PANIC even if we keep around the sanity check in
xlogrecovery_redo().  That would likely involve an immediate shutdown
of a standby followed by a start sequence?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
@ 2022-06-20 11:28               ` Kyotaro Horiguchi <[email protected]>
  2022-06-20 15:57                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-06-20 11:28 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers

At Mon, 20 Jun 2022 16:13:43 +0900, Michael Paquier <[email protected]> wrote in 
> On Fri, May 27, 2022 at 07:01:37PM +0000, Imseih (AWS), Sami wrote:
> > What we found:
> > 
> > 1. missingContrecPtr is set when 
> >    StandbyMode is false, therefore
> >    only a writer should set this value
> >    and a record is then sent downstream.
> > 
> >    But a standby going through crash 
> >    recovery will always have StandbyMode = false,
> >    causing the missingContrecPtr to be incorrectly
> >    set.
> 
> That stands as true as far as I know, StandbyMode would be switched
> only once we get out of crash recovery, and only if archive recovery
> completes when there is a restore_command.

Anyway the change;
-		abortedRecPtr = InvalidXLogRecPtr;
-		missingContrecPtr = InvalidXLogRecPtr;
+		//abortedRecPtr = InvalidXLogRecPtr;
+		//missingContrecPtr = InvalidXLogRecPtr;

Is injecting a bug that invalid "aborted contrecord" record can be
injected for certain conditions.  If a bug is intentionally injected,
it's quite natrual that some behavior gets broken..

> > 2. If StandbyModeRequested is checked instead,
> >      we ensure that a standby will not set a 
> >      missingContrecPtr.
> > 
> > 3. After applying the patch below, the tap test succeeded
> 
> Hmm.  I have not looked at that in depth, but if the intention is to
> check that the database is able to write WAL, looking at
> XLogCtl->SharedRecoveryState would be the way to go because that's the
> flip switching between crash recovery, archive recovery and the end of
> recovery (when WAL can be safely written).

What we are checking there is "we are going to write WAL", not "we are
writing".

(!StanbyMode && StandbyModeRequested) means the server have been
running crash-recovery before starting archive recovery.  In that
case, the server is supposed to continue with archived WAL without
insering a record.  However, if no archived WAL available and the
server promoted, the server needs to insert an "aborted contrecord"
record again.  (I'm not sure how that case happens in the field,
though.)

So I don't think !StandbyModeRequested is the right thing
here. Actually the attached test fails with the fix.

> The check in xlogrecovery_redo() still looks like a good thing to have
> anyway, because we know that we can safely skip the contrecord.  Now,
> for any patch produced, could the existing TAP test be extended so as
> we are able to get a PANIC even if we keep around the sanity check in
> xlogrecovery_redo().  That would likely involve an immediate shutdown
> of a standby followed by a start sequence?

Thus, I still don't see what have happened at Imseih's hand, but I can
cause PANIC with a bit tricky steps, which I don't think valid.  This
is what I wanted to know the exact steps to cause the PANIC.

The attached 1 is the PoC of the TAP test (it uses system()..), and
the second is a tentative fix for that.  (I don't like the fix, too,
though...)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Attachments:

  [text/x-patch] detect_aborted_contrec_panic.diff (1.2K, ../../[email protected]/2-detect_aborted_contrec_panic.diff)
  download | inline diff:
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 78feccd9aa..bce3d6d701 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -68,8 +68,7 @@ ok($initfile ne $endfile, "$initfile differs from $endfile");
 # contents
 $node->stop('immediate');
 
-unlink $node->basedir . "/pgdata/pg_wal/$endfile"
-  or die "could not unlink " . $node->basedir . "/pgdata/pg_wal/$endfile: $!";
+system(sprintf("mv %s/pgdata/pg_wal/$endfile %s/archives/", $node->basedir, $node->basedir));
 
 # OK, create a standby at this spot.
 $node->backup_fs_cold('backup');
@@ -106,4 +105,17 @@ $node_standby->promote;
 $node->stop;
 $node_standby->stop;
 
+my $node_primary_2  = PostgreSQL::Test::Cluster->new('primary_2');
+$node_primary_2->init_from_backup($node, 'backup', has_restoring => 1, standby => 0);
+
+$node_primary_2->append_conf(
+	'postgresql.conf', qq(
+log_min_messages = debug1
+));
+$node_primary_2->start;
+$node_primary_2->poll_query_until('postgres',
+								  'SELECT NOT pg_is_in_recovery()');
+is($node_primary_2->safe_psql('postgres', 'SELECT pg_is_in_recovery()'), 'f',
+  'check if the copied server has promoted');
+  
 done_testing();


  [text/x-patch] aborted_contrec_reset.diff (833B, ../../[email protected]/3-aborted_contrec_reset.diff)
  download | inline diff:
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626420..6b026af74e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3025,6 +3025,19 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
 
 		if (record)
 		{
+			/*
+			 * If we see an complete record after recorded aborted contrecord,
+			 * that means the aborted contrecord is a false one.  This happens
+			 * when we can continue archive recovery after crash recovery ended
+			 * with aborted contrecord.
+			 */
+			if (abortedRecPtr != InvalidXLogRecPtr &&
+				abortedRecPtr < xlogreader->EndRecPtr)
+			{
+				abortedRecPtr = InvalidXLogRecPtr;
+				missingContrecPtr = InvalidXLogRecPtr;
+			}
+
 			/* Great, got a record */
 			return record;
 		}


^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-06-20 15:57                 ` Robert Haas <[email protected]>
  2022-06-21 01:35                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Robert Haas @ 2022-06-20 15:57 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: Michael Paquier <[email protected]>; Imseih (AWS), Sami <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers

On Mon, Jun 20, 2022 at 7:28 AM Kyotaro Horiguchi
<[email protected]> wrote:
> > Hmm.  I have not looked at that in depth, but if the intention is to
> > check that the database is able to write WAL, looking at
> > XLogCtl->SharedRecoveryState would be the way to go because that's the
> > flip switching between crash recovery, archive recovery and the end of
> > recovery (when WAL can be safely written).
>
> What we are checking there is "we are going to write WAL", not "we are
> writing".
>
> (!StanbyMode && StandbyModeRequested) means the server have been
> running crash-recovery before starting archive recovery.  In that
> case, the server is supposed to continue with archived WAL without
> insering a record.  However, if no archived WAL available and the
> server promoted, the server needs to insert an "aborted contrecord"
> record again.  (I'm not sure how that case happens in the field,
> though.)
>
> So I don't think !StandbyModeRequested is the right thing
> here. Actually the attached test fails with the fix.

It seems to me that what we want to do is: if we're about to start
allowing WAL writes, then consider whether to insert an aborted
contrecord record. Now, if we are about to start allowing WAL write,
we must determine the LSN at which the first such record will be
written. Then there are two possibilities: either that LSN is on an
existing record boundary, or it isn't. In the former case, no aborted
contrecord record is required. In the latter case, we're writing at
LSN which would have been in the middle of a previous record, except
that the record in question was never finished or at least we don't
have all of it. So the first WAL we insert at our chosen starting LSN
must be an aborted contrecord record.

I'm not quite sure I understand the nature of the remaining bug here,
but this logic seems quite sketchy to me:

            /*
             * When not in standby mode we find that WAL ends in an incomplete
             * record, keep track of that record.  After recovery is done,
             * we'll write a record to indicate to downstream WAL readers that
             * that portion is to be ignored.
             */
            if (!StandbyMode &&
                !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
            {
                abortedRecPtr = xlogreader->abortedRecPtr;
                missingContrecPtr = xlogreader->missingContrecPtr;
            }

I don't see what StandbyMode has to do with anything here. The
question is whether the place where we're going to begin writing WAL
is on an existing record boundary or not. If it so happens that when
StandbyMode is turned on we can never decide to promote in the middle
of a record, then maybe there's no need to track this information when
StandbyMode = true, but I don't see a good reason why we should make
that assumption. What if in the future somebody added a command that
says "don't keep trying to read more WAL, just promote RIGHT HERE?". I
think this logic would surely be incorrect in that case. It feels to
me like the right thing to do is to always keep track if WAL ends in
an incomplete record, and then when we promote, we write an aborted
contrecord record if WAL ended in an incomplete record, full stop.

Now, we do need to keep in mind that, while in StandbyMode, we might
reach the end of WAL more than once, because we have a polling loop
that looks for more WAL. So it does not work to just set the values
once and then assume that's the whole truth forever. But why not
handle that by storing the abortedRecPtr and missingContrecPtr
unconditionally after every call to XLogPrefetcherReadRecord()? They
will go back and forth between XLogRecPtrIsInvalid and other values
many times but the last value should -- I think -- be however things
ended up at the point where we decided to stop reading WAL.

I haven't really dived into this issue much so I might be totally off
base, but it just doesn't feel right to me that this should depend on
whether we're in standby mode. That seems at best incidentally related
to whether an aborted contrecord record is required.

P.S. "aborted contrecord record" is really quite an awkward turn of phrase!

--
Robert Haas
EDB: http://www.enterprisedb.com





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-20 15:57                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
@ 2022-06-21 01:35                   ` Kyotaro Horiguchi <[email protected]>
  2022-06-22 01:41                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-22 14:38                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-06-21 01:35 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers

At Mon, 20 Jun 2022 11:57:20 -0400, Robert Haas <[email protected]> wrote in 
> It seems to me that what we want to do is: if we're about to start
> allowing WAL writes, then consider whether to insert an aborted
> contrecord record. Now, if we are about to start allowing WAL write,
> we must determine the LSN at which the first such record will be
> written. Then there are two possibilities: either that LSN is on an
> existing record boundary, or it isn't. In the former case, no aborted
> contrecord record is required. In the latter case, we're writing at
> LSN which would have been in the middle of a previous record, except
> that the record in question was never finished or at least we don't
> have all of it. So the first WAL we insert at our chosen starting LSN
> must be an aborted contrecord record.

Right.

> I'm not quite sure I understand the nature of the remaining bug here,
> but this logic seems quite sketchy to me:
> 
>             /*
>              * When not in standby mode we find that WAL ends in an incomplete
>              * record, keep track of that record.  After recovery is done,
>              * we'll write a record to indicate to downstream WAL readers that
>              * that portion is to be ignored.
>              */
>             if (!StandbyMode &&
>                 !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
>             {
>                 abortedRecPtr = xlogreader->abortedRecPtr;
>                 missingContrecPtr = xlogreader->missingContrecPtr;
>             }
> 
> I don't see what StandbyMode has to do with anything here. The
> question is whether the place where we're going to begin writing WAL
> is on an existing record boundary or not. If it so happens that when
> StandbyMode is turned on we can never decide to promote in the middle
> of a record, then maybe there's no need to track this information when
> StandbyMode = true, but I don't see a good reason why we should make
> that assumption. What if in the future somebody added a command that

Right.  I came to the same conclusion before reading this section. It
is rearer than other cases but surely possible.

> says "don't keep trying to read more WAL, just promote RIGHT HERE?". I
> think this logic would surely be incorrect in that case. It feels to
> me like the right thing to do is to always keep track if WAL ends in
> an incomplete record, and then when we promote, we write an aborted
> contrecord record if WAL ended in an incomplete record, full stop.

Agreed. Actually, with the second patch applied, removing !StandbyMode
from the condition makes no difference in behavior.

> Now, we do need to keep in mind that, while in StandbyMode, we might
> reach the end of WAL more than once, because we have a polling loop
> that looks for more WAL. So it does not work to just set the values
> once and then assume that's the whole truth forever. But why not
> handle that by storing the abortedRecPtr and missingContrecPtr
> unconditionally after every call to XLogPrefetcherReadRecord()? They
> will go back and forth between XLogRecPtrIsInvalid and other values
> many times but the last value should -- I think -- be however things
> ended up at the point where we decided to stop reading WAL.

Unfortunately it doesn't work because we read a record already known
to be complete again at the end of recovery.  It is the reason of
"abortedRecPtr < xlogreader->EndRecPtr" in my PoC patch.  Without it,
abrotedRecPtr is erased when it is actually needed.  I don't like that
expedient-looking condition, but the strict condition for resetting
abortedRecPtr is iff "we have read a complete record at the same or
grater LSN ofabortedRecPtr"...

Come to think of this, I noticed that we can get rid of the
file-global abortedRecPtr by letting FinishWalRecovery copy
abortedRecPtr *before* reading the last record. This also allows us to
remove the "expedient" condition.


> I haven't really dived into this issue much so I might be totally off
> base, but it just doesn't feel right to me that this should depend on
> whether we're in standby mode. That seems at best incidentally related
> to whether an aborted contrecord record is required.
> 
> P.S. "aborted contrecord record" is really quite an awkward turn of phrase!

Thats true!  Always open for better phrasings:(

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Attachments:

  [text/x-patch] aborted_contrec_reset_3.patch (3.1K, ../../[email protected]/2-aborted_contrec_reset_3.patch)
  download | inline diff:
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626420..c1be15039d 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -360,15 +360,6 @@ typedef struct XLogRecoveryCtlData
 
 static XLogRecoveryCtlData *XLogRecoveryCtl = NULL;
 
-/*
- * abortedRecPtr is the start pointer of a broken record at end of WAL when
- * recovery completes; missingContrecPtr is the location of the first
- * contrecord that went missing.  See CreateOverwriteContrecordRecord for
- * details.
- */
-static XLogRecPtr abortedRecPtr;
-static XLogRecPtr missingContrecPtr;
-
 /*
  * if recoveryStopsBefore/After returns true, it saves information of the stop
  * point here
@@ -944,12 +935,6 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 		minRecoveryPointTLI = 0;
 	}
 
-	/*
-	 * Start recovery assuming that the final record isn't lost.
-	 */
-	abortedRecPtr = InvalidXLogRecPtr;
-	missingContrecPtr = InvalidXLogRecPtr;
-
 	*wasShutdown_ptr = wasShutdown;
 	*haveBackupLabel_ptr = haveBackupLabel;
 	*haveTblspcMap_ptr = haveTblspcMap;
@@ -1430,6 +1415,14 @@ FinishWalRecovery(void)
 		lastRec = XLogRecoveryCtl->lastReplayedReadRecPtr;
 		lastRecTLI = XLogRecoveryCtl->lastReplayedTLI;
 	}
+
+	/*
+	 *  record aborted contrecord status before the next ReadRecord erases the
+	 *  state
+	 */
+	result->abortedRecPtr = xlogreader->abortedRecPtr;
+	result->missingContrecPtr = xlogreader->missingContrecPtr;
+
 	XLogPrefetcherBeginRead(xlogprefetcher, lastRec);
 	(void) ReadRecord(xlogprefetcher, PANIC, false, lastRecTLI);
 	endOfLog = xlogreader->EndRecPtr;
@@ -1503,9 +1496,6 @@ FinishWalRecovery(void)
 	result->lastRecTLI = lastRecTLI;
 	result->endOfLog = endOfLog;
 
-	result->abortedRecPtr = abortedRecPtr;
-	result->missingContrecPtr = missingContrecPtr;
-
 	result->standby_signal_file_found = standby_signal_file_found;
 	result->recovery_signal_file_found = recovery_signal_file_found;
 
@@ -1970,10 +1960,6 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
 				 LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
 				 LSN_FORMAT_ARGS(record->overwrittenRecPtr));
 
-		/* We have safely skipped the aborted record */
-		abortedRecPtr = InvalidXLogRecPtr;
-		missingContrecPtr = InvalidXLogRecPtr;
-
 		ereport(LOG,
 				(errmsg("successfully skipped missing contrecord at %X/%X, overwritten at %s",
 						LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
@@ -2971,19 +2957,6 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
 		record = XLogPrefetcherReadRecord(xlogprefetcher, &errormsg);
 		if (record == NULL)
 		{
-			/*
-			 * When not in standby mode we find that WAL ends in an incomplete
-			 * record, keep track of that record.  After recovery is done,
-			 * we'll write a record to indicate to downstream WAL readers that
-			 * that portion is to be ignored.
-			 */
-			if (!StandbyMode &&
-				!XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
-			{
-				abortedRecPtr = xlogreader->abortedRecPtr;
-				missingContrecPtr = xlogreader->missingContrecPtr;
-			}
-
 			if (readFile >= 0)
 			{
 				close(readFile);


^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-20 15:57                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  2022-06-21 01:35                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-06-22 01:41                     ` Michael Paquier <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Michael Paquier @ 2022-06-22 01:41 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers

On Tue, Jun 21, 2022 at 10:35:33AM +0900, Kyotaro Horiguchi wrote:
> At Mon, 20 Jun 2022 11:57:20 -0400, Robert Haas <[email protected]> wrote in 
>> says "don't keep trying to read more WAL, just promote RIGHT HERE?". I
>> think this logic would surely be incorrect in that case. It feels to
>> me like the right thing to do is to always keep track if WAL ends in
>> an incomplete record, and then when we promote, we write an aborted
>> contrecord record if WAL ended in an incomplete record, full stop.

Agreed.  The state is maintained in the WAL reader as far as I
understand the logic behind it.

>> Now, we do need to keep in mind that, while in StandbyMode, we might
>> reach the end of WAL more than once, because we have a polling loop
>> that looks for more WAL. So it does not work to just set the values
>> once and then assume that's the whole truth forever. But why not
>> handle that by storing the abortedRecPtr and missingContrecPtr
>> unconditionally after every call to XLogPrefetcherReadRecord()? They
>> will go back and forth between XLogRecPtrIsInvalid and other values
>> many times but the last value should -- I think -- be however things
>> ended up at the point where we decided to stop reading WAL.
> 
> Unfortunately it doesn't work because we read a record already known
> to be complete again at the end of recovery.  It is the reason of
> "abortedRecPtr < xlogreader->EndRecPtr" in my PoC patch.  Without it,
> abrotedRecPtr is erased when it is actually needed.  I don't like that
> expedient-looking condition, but the strict condition for resetting
> abortedRecPtr is iff "we have read a complete record at the same or
> grater LSN ofabortedRecPtr"...
> 
> Come to think of this, I noticed that we can get rid of the
> file-global abortedRecPtr by letting FinishWalRecovery copy
> abortedRecPtr *before* reading the last record. This also allows us to
> remove the "expedient" condition.

Interesting point, though I am not sure to get why this is safe
compared to the existing solution of setting
missingContrecPtr/abortedRecPtr while reading a set of records when we
look for the last LSN at the end of recovery.

>> I haven't really dived into this issue much so I might be totally off
>> base, but it just doesn't feel right to me that this should depend on
>> whether we're in standby mode. That seems at best incidentally related
>> to whether an aborted contrecord record is required.
>> 
>> P.S. "aborted contrecord record" is really quite an awkward turn of phrase!
> 
> Thats true!  Always open for better phrasings:(

The term that would be appropriate here is continuation record?
contrecord is a bit confusing for French-speakers, actually, as
"contre" means "against" ;)

By the way, something that itches me with the proposed patch is that
we don't actually stress the second problem reported, which is that
the use of StandbyMode is incorrect.  Isn't that a sign that we'd
better extend more the tests of 026_overwrite_contrecord.pl with more
end-of-recovery scenarios?  Two things that immediately come to mind
are the use of recovery_target_lsn that would force a promotion in the
middle of a continuation record and cascading standbys to make sure
that these get the extra OVERWRITE_CONTRECORD record generated at the
end of recovery.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-20 15:57                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  2022-06-21 01:35                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-06-22 14:38                     ` Robert Haas <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Robert Haas @ 2022-06-22 14:38 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: Michael Paquier <[email protected]>; Imseih (AWS), Sami <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers

On Mon, Jun 20, 2022 at 9:35 PM Kyotaro Horiguchi
<[email protected]> wrote:
> Unfortunately it doesn't work because we read a record already known
> to be complete again at the end of recovery.  It is the reason of
> "abortedRecPtr < xlogreader->EndRecPtr" in my PoC patch.  Without it,
> abrotedRecPtr is erased when it is actually needed.  I don't like that
> expedient-looking condition, but the strict condition for resetting
> abortedRecPtr is iff "we have read a complete record at the same or
> grater LSN ofabortedRecPtr"...

Yeah, we need to work around that somehow. I noticed the
record-rereading behavior when I was working on some patches for this
release cycle and I think what we ought to do is get rid of that. It
serves no purpose other than to make things complicated. However, we
shouldn't back-patch a change like that, I think, so we'll need to
work around the issue somehow. Perhaps it'd be better to return these
values to the caller somehow and then the caller can decide whether to
save them based on context. The last re-read can choose not to do so.

-- 
Robert Haas
EDB: http://www.enterprisedb.com





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-06-24 16:17                 ` Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-06-24 16:17 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers

>    Thus, I still don't see what have happened at Imseih's hand, but I can
>    cause PANIC with a bit tricky steps, which I don't think valid.  This
>    is what I wanted to know the exact steps to cause the PANIC.

>    The attached 1 is the PoC of the TAP test (it uses system()..), and
>    the second is a tentative fix for that.  (I don't like the fix, too,
>    though...)

It is been difficult to get a generic repro, but the way we reproduce
Is through our test suite. To give more details, we are running tests
In which we constantly failover and promote standbys. The issue
surfaces after we have gone through a few promotions which occur
every few hours or so ( not really important but to give context ).

I am adding some additional debugging  to see if I can draw a better
picture of what is happening. Will also give aborted_contrec_reset_3.patch 
a go, although I suspect it will not handle the specific case we are deaing with.


Regards,

Sami imseih
Amazon Web Services



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-06-27 06:02                   ` Michael Paquier <[email protected]>
  2022-06-28 00:46                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Michael Paquier @ 2022-06-27 06:02 UTC (permalink / raw)
  To: Imseih (AWS), Sami <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; pgsql-hackers

On Fri, Jun 24, 2022 at 04:17:34PM +0000, Imseih (AWS), Sami wrote:
> It is been difficult to get a generic repro, but the way we reproduce
> Is through our test suite. To give more details, we are running tests
> In which we constantly failover and promote standbys. The issue
> surfaces after we have gone through a few promotions which occur
> every few hours or so ( not really important but to give context ).

Hmm.  Could you describe exactly the failover scenario you are using?
Is the test using a set of cascading standbys linked to the promoted
one?  Are the standbys recycled from the promoted nodes with pg_rewind
or created from scratch with a new base backup taken from the
freshly-promoted primary?  I have been looking more at this thread
through the day but I don't see a remaining issue.  It could be
perfectly possible that we are missing a piece related to the handling
of those new overwrite contrecords in some cases, like in a rewind.

> I am adding some additional debugging  to see if I can draw a better
> picture of what is happening. Will also give aborted_contrec_reset_3.patch 
> a go, although I suspect it will not handle the specific case we are deaing with.

Yeah, this is not going to change much things if you are still seeing
an issue.  This patch does not change the logic, aka it just
simplifies the tracking of the continuation record data, resetting it
when a complete record has been read.  Saying that, getting rid of the
dependency on StandbyMode because we cannot promote in the middle of a
record is nice (my memories around that were a bit blurry but even
recovery_target_lsn would not recover in the middle of an continuation
record), and this is not bug so there is limited reason to backpatch
this part of the change.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../YrlH47KV%[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
@ 2022-06-28 00:46                     ` Kyotaro Horiguchi <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-06-28 00:46 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers

At Mon, 27 Jun 2022 15:02:11 +0900, Michael Paquier <[email protected]> wrote in 
> On Fri, Jun 24, 2022 at 04:17:34PM +0000, Imseih (AWS), Sami wrote:
> > It is been difficult to get a generic repro, but the way we reproduce
> > Is through our test suite. To give more details, we are running tests
> > In which we constantly failover and promote standbys. The issue
> > surfaces after we have gone through a few promotions which occur
> > every few hours or so ( not really important but to give context ).
> 
> Hmm.  Could you describe exactly the failover scenario you are using?
> Is the test using a set of cascading standbys linked to the promoted
> one?  Are the standbys recycled from the promoted nodes with pg_rewind
> or created from scratch with a new base backup taken from the
> freshly-promoted primary?  I have been looking more at this thread
> through the day but I don't see a remaining issue.  It could be
> perfectly possible that we are missing a piece related to the handling
> of those new overwrite contrecords in some cases, like in a rewind.
> 
> > I am adding some additional debugging  to see if I can draw a better
> > picture of what is happening. Will also give aborted_contrec_reset_3.patch 
> > a go, although I suspect it will not handle the specific case we are deaing with.
> 
> Yeah, this is not going to change much things if you are still seeing
> an issue.  This patch does not change the logic, aka it just

True. That is a siginicant hint on what happened at the time.

- Are there only two hosts in the replication set?  I concerned on
  whether it is a cascading set or not.

- Exactly what are you performing at every failover?  Especially do
  the steps contain pg_rewind, and do you copy pg_wal and/or archive
  files between the failover hosts?

> simplifies the tracking of the continuation record data, resetting it
> when a complete record has been read.  Saying that, getting rid of the
> dependency on StandbyMode because we cannot promote in the middle of a
> record is nice (my memories around that were a bit blurry but even
> recovery_target_lsn would not recover in the middle of an continuation
> record), and this is not bug so there is limited reason to backpatch
> this part of the change.

Agreed.  In the first place my "repro" (or the test case) is a bit too
intricated to happen in the real field.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
@ 2022-06-28 07:09                     ` Kyotaro Horiguchi <[email protected]>
  2022-06-29 13:47                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  1 sibling, 2 replies; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-06-28 07:09 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers

I'd like to look into the WAL segments related to the failure.

Mmm... With the patch, xlogreader->abortedRecPtr is valid only and
always when the last read failed record was an aborted contrec. If
recovery ends here the first insereted record is an "aborted contrec"
record.  I still see it as the only chance that an aborted contrecord
is followed by a non-"aborted contrec" record is that recovery somehow
fetches two consecutive WAL segments that are inconsistent at the
boundary.


I found the reason that the TAP test doesn't respond to the first
proposed patch (the below).

-			if (!StandbyMode &&
+			if (!StandbyModeRequested &&
				!XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))

The cause was that I disabled standby-mode in the test. The change
affects only while standby mode is on, which was to make the test
reliable and simpler. The first attached detects the same error (in a
somwhat maybe-unstable way) and responds to the fix above, and also
responds to the aborted_contrec_reset_3.patch.

So, aborted_contrec_reset_3 looks closer to the issue than before.

Would you mind trying the second attached to abtain detailed log on
your testing environment? With the patch, the modified TAP test yields
the log lines like below.

2022-06-28 15:49:20.661 JST [165472] LOG:  ### [A] @0/1FFD338: abort=(0/1FFD338)0/0, miss=(0/2000000)0/0, SbyMode=0, SbyModeReq=1
...
2022-06-28 15:49:20.681 JST [165472] LOG:  ### [F] @0/2094610: abort=(0/0)0/1FFD338, miss=(0/0)0/2000000, SbyMode=1, SbyModeReq=1
...
2022-06-28 15:49:20.767 JST [165472] LOG:  ### [S] @0/2094610: abort=(0/0)0/1FFD338, miss=(0/0)0/2000000, SbyMode=0, SbyModeReq=1
...
2022-06-28 15:49:20.777 JST [165470] PANIC:  xlog flush request 0/2094610 is not satisfied --- flushed only to 0/2000088

In this example, abortedRecPtr is set at the first line and recovery
continued to 2094610 but abortedRecPtr is not reset then PANICed. ([A]
means aborted contrec falure. [F] and [S] are failed and succeeded
reads respectively.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Attachments:

  [text/x-patch] detect_aborted_contrec_panic_2.diff (1.8K, ../../[email protected]/2-detect_aborted_contrec_panic_2.diff)
  download | inline diff:
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 78feccd9aa..043eff86ec 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -59,6 +59,7 @@ $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 $endlsn = $node->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn()');
 my $endfile = $node->safe_psql('postgres',
 	'SELECT pg_walfile_name(pg_current_wal_insert_lsn())');
 ok($initfile ne $endfile, "$initfile differs from $endfile");
@@ -68,9 +69,8 @@ ok($initfile ne $endfile, "$initfile differs from $endfile");
 # contents
 $node->stop('immediate');
 
-unlink $node->basedir . "/pgdata/pg_wal/$endfile"
-  or die "could not unlink " . $node->basedir . "/pgdata/pg_wal/$endfile: $!";
-
+system(sprintf("mv %s/pgdata/pg_wal/$endfile %s/archives/", $node->basedir, $node->basedir));
+ 
 # OK, create a standby at this spot.
 $node->backup_fs_cold('backup');
 my $node_standby = PostgreSQL::Test::Cluster->new('standby');
@@ -106,4 +106,17 @@ $node_standby->promote;
 $node->stop;
 $node_standby->stop;
 
+my $node_primary_2  = PostgreSQL::Test::Cluster->new('primary_2');
+$node_primary_2->init_from_backup($node, 'backup', has_restoring => 1, standby => 1);
+
+$node_primary_2->append_conf(
+	'postgresql.conf', qq(
+log_min_messages = debug1
+));
+$node_primary_2->start;
+$node_primary_2->poll_query_until('postgres',
+					  "SELECT '$endlsn'::pg_lsn <= pg_last_wal_replay_lsn()");
+$node_primary_2->promote;
+$node_primary_2->poll_query_until('postgres', 'SELECT NOT pg_is_in_recovery()');
+$node_primary_2->safe_psql('postgres', 'CREATE TABLE bar AS select generate_series(0, 99999) a');
 done_testing();


  [text/x-patch] abortcont_additional_log.diff (959B, ../../[email protected]/3-abortcont_additional_log.diff)
  download | inline diff:
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626420..cd02e6d8dd 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2967,8 +2967,17 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
 	for (;;)
 	{
 		char	   *errormsg;
+		static int log = 2;
 
 		record = XLogPrefetcherReadRecord(xlogprefetcher, &errormsg);
+
+		if (!record)
+			log = 2;
+		else if (log > 0)
+			log--;
+
+		if (log)
+			elog(LOG, "### [%s] @%X/%X: abort=(%X/%X)%X/%X, miss=(%X/%X)%X/%X, SbyMode=%d, SbyModeReq=%d", record ? "S": (XLogRecPtrIsInvalid(xlogreader->abortedRecPtr) ? "F" : "A"), LSN_FORMAT_ARGS(xlogreader->EndRecPtr), LSN_FORMAT_ARGS(xlogreader->abortedRecPtr), LSN_FORMAT_ARGS(abortedRecPtr), LSN_FORMAT_ARGS(xlogreader->missingContrecPtr), LSN_FORMAT_ARGS(missingContrecPtr), StandbyMode, StandbyModeRequested); 
 		if (record == NULL)
 		{
 			/*


^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-06-29 13:47                       ` Imseih (AWS), Sami <[email protected]>
  2022-07-28 08:02                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion [email protected] <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-06-29 13:47 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers

> Would you mind trying the second attached to abtain detailed log on
> your testing environment? With the patch, the modified TAP test yields
> the log lines like below.

Thanks for this. I will apply this to the testing environment and
will share the output.

Regards,

Sami Imseih
Amazon Web Services (AWS)



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-29 13:47                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-07-28 08:02                         ` [email protected] <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: [email protected] @ 2022-07-28 08:02 UTC (permalink / raw)
  To: Imseih (AWS), Sami <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; pgsql-hackers

Hello,

On 2022-Jun-29, Imseih (AWS), Sami wrote:

> > Would you mind trying the second attached to abtain detailed log on
> > your testing environment? With the patch, the modified TAP test yields
> > the log lines like below.
> 
> Thanks for this. I will apply this to the testing environment and
> will share the output.

Any luck with this?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-07-29 15:07                       ` Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-07-29 15:07 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers

> Any luck with this?

Apologies for the delay, as I have been away. 
I will test this next week and report back my findings.

Thanks

Sami Imseih
Amazon Web Services (AWS)


On 6/28/22, 2:10 AM, "Kyotaro Horiguchi" <[email protected]> wrote:

    CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



    I'd like to look into the WAL segments related to the failure.

    Mmm... With the patch, xlogreader->abortedRecPtr is valid only and
    always when the last read failed record was an aborted contrec. If
    recovery ends here the first insereted record is an "aborted contrec"
    record.  I still see it as the only chance that an aborted contrecord
    is followed by a non-"aborted contrec" record is that recovery somehow
    fetches two consecutive WAL segments that are inconsistent at the
    boundary.


    I found the reason that the TAP test doesn't respond to the first
    proposed patch (the below).

    -                       if (!StandbyMode &&
    +                       if (!StandbyModeRequested &&
                                    !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))

    The cause was that I disabled standby-mode in the test. The change
    affects only while standby mode is on, which was to make the test
    reliable and simpler. The first attached detects the same error (in a
    somwhat maybe-unstable way) and responds to the fix above, and also
    responds to the aborted_contrec_reset_3.patch.

    So, aborted_contrec_reset_3 looks closer to the issue than before.

    Would you mind trying the second attached to abtain detailed log on
    your testing environment? With the patch, the modified TAP test yields
    the log lines like below.

    2022-06-28 15:49:20.661 JST [165472] LOG:  ### [A] @0/1FFD338: abort=(0/1FFD338)0/0, miss=(0/2000000)0/0, SbyMode=0, SbyModeReq=1
    ...
    2022-06-28 15:49:20.681 JST [165472] LOG:  ### [F] @0/2094610: abort=(0/0)0/1FFD338, miss=(0/0)0/2000000, SbyMode=1, SbyModeReq=1
    ...
    2022-06-28 15:49:20.767 JST [165472] LOG:  ### [S] @0/2094610: abort=(0/0)0/1FFD338, miss=(0/0)0/2000000, SbyMode=0, SbyModeReq=1
    ...
    2022-06-28 15:49:20.777 JST [165470] PANIC:  xlog flush request 0/2094610 is not satisfied --- flushed only to 0/2000088

    In this example, abortedRecPtr is set at the first line and recovery
    continued to 2094610 but abortedRecPtr is not reset then PANICed. ([A]
    means aborted contrec falure. [F] and [S] are failed and succeeded
    reads respectively.)

    regards.

    --
    Kyotaro Horiguchi
    NTT Open Source Software Center



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-08-05 21:28                         ` Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-08-05 21:28 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers

> Would you mind trying the second attached to abtain detailed log on
> your testing environment? With the patch, the modified TAP test yields
> the log lines like below.

I applied the logging patch to 13.7 ( attached is the backport ) and repro'd the 
Issue.

I stripped out the relevant parts of the file. Let me know if this is
helpful.

postgresql.log.2022-08-05-17:2022-08-05 17:18:51 UTC::@:[359]:LOG:  ### [S] @0/48A3400: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
postgresql.log.2022-08-05-17:2022-08-05 17:18:51 UTC::@:[359]:LOG:  ### [F] @0/10000000: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
postgresql.log.2022-08-05-17:2022-08-05 17:22:21 UTC::@:[359]:LOG:  ### [S] @0/10000060: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:00 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:05 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:06 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:11 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [S] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:50:13 UTC::@:[359]:LOG:  ### [S] @6/B8000198: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:50:20 UTC::@:[359]:LOG:  ### [A] @6/F3FFFF20: abort=(6/F3FFFF20)0/0, miss=(6/F4000000)0/0, SbyMode=0, SbyModeReq=1
postgresql.log.2022-08-05-18:2022-08-05 18:50:20 UTC::@:[359]:LOG:  ### [S] @6/F4000030: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:06 UTC::@:[359]:LOG:  ### [F] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:11 UTC::@:[359]:LOG:  ### [F] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:13 UTC::@:[359]:LOG:  ### [F] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:18 UTC::@:[359]:LOG:  ### [F] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:21 UTC::@:[359]:LOG:  ### [F] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1
postgresql.log.2022-08-05-20:2022-08-05 20:38:21 UTC::@:[359]:LOG:  ### [S] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=0, SbyModeReq=1
....
.......
.........

2022-08-05 20:38:21 UTC::@:[359]:LOG:  received promote request
2022-08-05 20:38:21 UTC::@:[359]:LOG:  redo done at E/17A78270
2022-08-05 20:38:21 UTC::@:[359]:LOG:  last completed transaction was at log time 2022-08-05 20:38:06.398382+00
recovering 000000020000000E00000005
000000020000000E00000005 archive /rdsdbdata/log/restore/pg-wal-archive.901.* is not yet downloaded, exiting restore script for now
2022-08-05 20:38:21 UTC::@:[359]:LOG:  ### [S] @E/17A78298: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=0, SbyModeReq=1
recovering 00000003.history
2022-08-05 20:38:21 UTC::@:[359]:LOG:  selected new timeline ID: 3
2022-08-05 20:38:21 UTC::@:[359]:LOG:  archive recovery complete
recovering 00000002.history
2022-08-05 20:38:22 UTC::@:[373]:LOG:  request to flush past end of generated WAL; request D/C7A425C0, currpos 6/F4000088
2022-08-05 20:38:22 UTC::@:[373]:CONTEXT:  writing block 0 of relation base/14301/26678
2022-08-05 20:38:22 UTC::@:[373]:ERROR:  xlog flush request D/C7A425C0 is not satisfied --- flushed only to 6/F4000088
2022-08-05 20:38:22 UTC::@:[373]:CONTEXT:  writing block 0 of relation base/14301/26678
2022-08-05 20:38:22 UTC::@:[357]:LOG:  database system is ready to accept connections
2022-08-05 20:38:23 UTC::@:[373]:LOG:  checkpoint starting: immediate force wait wal time
2022-08-05 20:38:23 UTC::@:[373]:LOG:  request to flush past end of generated WAL; request D/F5873CF0, currpos 6/F8000060
2022-08-05 20:38:23 UTC::@:[373]:PANIC:  xlog flush request D/F5873CF0 is not satisfied --- flushed only to 6/F8000060
2022-08-05 20:38:23 UTC::@:[357]:LOG:  checkpointer process (PID 373) was terminated by signal 6: Aborted
2022-08-05 20:38:23 UTC::@:[357]:LOG:  terminating any other active server processes
/etc/rds/dbbin/pgscripts/rds_wal_archive: line 19: 14546 Quit                    (core dumped) usleep 50000
2022-08-05 20:38:23 UTC::@:[375]:FATAL:  archive command failed with exit code 131
2022-08-05 20:38:23 UTC::@:[375]:DETAIL:  The failed archive command was: /etc/rds/dbbin/pgscripts/rds_wal_archive pg_wal/00000003000000060000003D


Thanks,

Sami Imseih
Amazon Web Services




Attachments:

  [application/octet-stream] abortcont_additional_log_137.diff (1.5K, ../../[email protected]/2-abortcont_additional_log_137.diff)
  download | inline diff:
From e5b5c2e933d4cfb2759e5e5da7c2f6b6b597b9bf Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)" <[email protected]>
Date: Thu, 4 Aug 2022 16:39:34 +0000
Subject: [PATCH 1/1] DEBUG suggestions by community

---
 src/backend/access/transam/xlog.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c03eaf0ed5..4169f03523 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4398,8 +4398,27 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 	for (;;)
 	{
 		char	   *errormsg;
+		static int log = 2;
 
 		record = XLogReadRecord(xlogreader, &errormsg);
+		if (!record)
+			log = 2;
+		else if (log > 0)
+			log--;
+
+		if (log)
+			elog(LOG,
+				"### [%s] @%X/%X: abort=(%X/%X)%X/%X, miss=(%X/%X)%X/%X, SbyMode=%d, SbyModeReq=%d",
+				record ? "S": (XLogRecPtrIsInvalid(xlogreader->abortedRecPtr) ? "F" : "A"),
+					(uint32) (xlogreader->EndRecPtr >> 32), (uint32) xlogreader->EndRecPtr,
+					(uint32) (xlogreader->abortedRecPtr >>32), (uint32) xlogreader->abortedRecPtr,
+					(uint32) (abortedRecPtr >>32), (uint32) abortedRecPtr,
+					(uint32) (xlogreader->missingContrecPtr >>32),
+								(uint32) xlogreader->missingContrecPtr,
+					(uint32) (missingContrecPtr >>32), (uint32) missingContrecPtr,
+				StandbyMode,
+				StandbyModeRequested);
+
 		ReadRecPtr = xlogreader->ReadRecPtr;
 		EndRecPtr = xlogreader->EndRecPtr;
 		if (record == NULL)
-- 
2.37.1



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-08-08 04:06                           ` Kyotaro Horiguchi <[email protected]>
  2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-08-08 04:06 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers

At Fri, 5 Aug 2022 21:28:16 +0000, "Imseih (AWS), Sami" <[email protected]> wrote in 
> > Would you mind trying the second attached to abtain detailed log on
> > your testing environment? With the patch, the modified TAP test yields
> > the log lines like below.
> 
> I applied the logging patch to 13.7 ( attached is the backport ) and repro'd the 
> Issue.
> 
> I stripped out the relevant parts of the file. Let me know if this is
> helpful.

Thank you very much!

> postgresql.log.2022-08-05-17:2022-08-05 17:18:51 UTC::@:[359]:LOG:  ### [F] @0/10000000: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
> postgresql.log.2022-08-05-17:2022-08-05 17:22:21 UTC::@:[359]:LOG:  ### [S] @0/10000060: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1

The server seem to have started as a standby after crashing a
primary. Is it correct?

> postgresql.log.2022-08-05-18:2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
> postgresql.log.2022-08-05-18:2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [S] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1

Archive recovery ended here. The server should have promoted that
time..  Do you see some interesting log lines around this time?

> postgresql.log.2022-08-05-18:2022-08-05 18:50:13 UTC::@:[359]:LOG:  ### [S] @6/B8000198: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1

But, recovery continues in non-standby mode.  I don't see how come it
behaves that way.

> postgresql.log.2022-08-05-18:2022-08-05 18:50:20 UTC::@:[359]:LOG:  ### [A] @6/F3FFFF20: abort=(6/F3FFFF20)0/0, miss=(6/F4000000)0/0, SbyMode=0, SbyModeReq=1
> postgresql.log.2022-08-05-18:2022-08-05 18:50:20 UTC::@:[359]:LOG:  ### [S] @6/F4000030: abort=(0/0)6/F3FFFF20, miss=(0/0)6/F4000000, SbyMode=1, SbyModeReq=1

Then archive recovery starts again.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-08-08 15:51                             ` Imseih (AWS), Sami <[email protected]>
  2022-08-10 08:37                               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-08-08 15:51 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers

> The server seem to have started as a standby after crashing a
> primary. Is it correct?

Yes, that is correct.

2022-08-05 17:18:51 UTC::@:[359]:LOG:  database system was interrupted; last known up at 2022-08-05 17:08:52 UTC
2022-08-05 17:18:51 UTC::@:[359]:LOG:  creating missing WAL directory "pg_wal/archive_status"
recovering 00000002.history
2022-08-05 17:18:51 UTC::@:[359]:LOG:  entering standby mode
2022-08-05 17:18:51 UTC::@:[359]:LOG:  ### [S] @0/48A3400: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  checkpoint record is at 0/48A3388
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  redo record is at 0/48A3388; shutdown true
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  next transaction ID: 533; next OID: 16395
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  next MultiXactId: 1; next MultiXactOffset: 0
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  oldest unfrozen transaction ID: 479, in database 1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  oldest MultiXactId: 1, in database 1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  commit timestamp Xid oldest/newest: 0/0
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  transaction ID wrap limit is 2147484126, limited by database with OID 1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  MultiXactId wrap limit is 2147483648, limited by database with OID 1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  starting up replication slots
2022-08-05 17:18:51 UTC::@:[359]:LOG:  database system was not properly shut down; automatic recovery in progress
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  resetting unlogged relations: cleanup 1 init 0
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  initializing for hot standby
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  recovery snapshots are now enabled
2022-08-05 17:18:51 UTC::@:[359]:LOG:  redo starts at 0/48A3400
2022-08-05 17:18:51 UTC::@:[359]:LOG:  ### [F] @0/10000000: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
2022-08-05 17:18:51 UTC::@:[359]:DEBUG:  reached end of WAL in pg_wal, entering archive recovery
2022-08-05 17:18:51 UTC::@:[359]:LOG:  consistent recovery state reached at 0/10000000
2022-08-05 17:18:51 UTC::@:[357]:LOG:  database system is ready to accept read only connections


> Archive recovery ended here. The server should have promoted that
> time..  Do you see some interesting log lines around this time?

The server did promote around that time

2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [F] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=1, SbyModeReq=1
2022-08-05 18:38:14 UTC::@:[359]:LOG:  received promote request
2022-08-05 18:38:14 UTC::@:[359]:LOG:  redo done at 6/B6CB27A8
2022-08-05 18:38:14 UTC::@:[359]:LOG:  last completed transaction was at log time 2022-08-05 18:38:00.832047+00
recovering 00000001000000060000002D
....
........
2022-08-05 18:38:14 UTC::@:[359]:LOG:  ### [S] @6/B6CB27D0: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
recovering 00000002.history
2022-08-05 18:38:14 UTC::@:[359]:LOG:  selected new timeline ID: 2
2022-08-05 18:38:14 UTC::@:[359]:LOG:  archive recovery complete
recovering 00000001.history
2022-08-05 18:38:14 UTC::@:[357]:LOG:  database system is ready to accept connections
2022-08-05 18:38:15 UTC::@:[367]:LOG:  restartpoint complete: wrote 21388 buffers (2.1%); 0 WAL file(s) added, 9 removed, 0 recycled; write=98.394 s, sync=0.041 s, total=98.586 s; sync files=46, longest=0.012 s, average=0.001 s; distance=1048565 kB, estimate=1048584 kB
2022-08-05 18:38:15 UTC::@:[367]:LOG:  recovery restart point at 6/5C0003B0

>    But, recovery continues in non-standby mode.  I don't see how come it
>    behaves that way.

But the server crashes sometime after which is why recovery starts.

022-08-05 18:50:13 UTC::@:[357]:LOG:  listening on IPv4 address "0.0.0.0", port 5432
2022-08-05 18:50:13 UTC::@:[357]:LOG:  listening on IPv6 address "::", port 5432
2022-08-05 18:50:13 UTC::@:[357]:LOG:  listening on Unix socket "/tmp/.s.PGSQL.5432"
2022-08-05 18:50:13 UTC::@:[359]:LOG:  database system was interrupted; last known up at 2022-08-05 18:38:15 UTC
2022-08-05 18:50:13 UTC::@:[359]:LOG:  creating missing WAL directory "pg_wal/archive_status"
recovering 00000003.history
2022-08-05 18:50:13 UTC::@:[359]:LOG:  entering standby mode
recovering 00000002.history
2022-08-05 18:50:13 UTC::@:[359]:LOG:  ### [S] @6/B8000198: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
2022-08-05 18:50:13 UTC::@:[359]:LOG:  database system was not properly shut down; automatic recovery in progress
2022-08-05 18:50:13 UTC::@:[359]:LOG:  redo starts at 6/B80000E8

And a few hours later, is when we see a panic

Thanks

--
Sami Imseih
Amazon Web Services (AWS)




^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
@ 2022-08-10 08:37                               ` Kyotaro Horiguchi <[email protected]>
  2022-08-29 16:54                                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Kyotaro Horiguchi @ 2022-08-10 08:37 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers

Hello.

> Yes, that is correct.

Mmm. I believed that the log came from a single server run, since the
PID (I believe the [359], [357] are PID) did not change through the
log lines.

> 2022-08-05 18:50:13 UTC::@:[359]:LOG:  creating missing WAL directory "pg_wal/archive_status"

This means that someone removes the content of pg_wal directory.
Removing some WAL files in pg_wal may lead to this symptom.

> 2022-08-05 18:50:13 UTC::@:[359]:LOG:  entering standby mode
> recovering 00000002.history
> 2022-08-05 18:50:13 UTC::@:[359]:LOG:  ### [S] @6/B8000198: abort=(0/0)0/0, miss=(0/0)0/0, SbyMode=0, SbyModeReq=1
> 2022-08-05 18:50:13 UTC::@:[359]:LOG:  database system was not properly shut down; automatic recovery in progress
> 2022-08-05 18:50:13 UTC::@:[359]:LOG:  redo starts at 6/B80000E8
> 
> And a few hours later, is when we see a panic

So, it seems that the *standby* received the inconsistent WAL stream
(aborted-contrecord not followed by a overwriting-missing-contrecord)
from the primary.  Thus the inconsistency happened on the primary, not
on the standby.

So... I'm still failing to draw the big picutre of what is happening
here.

Could you show us the server configuration (dbservers involved and
their roles (primary/standby..)), and the exact steps when you restart
the server after carsh?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-10 08:37                               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
@ 2022-08-29 16:54                                 ` Robert Haas <[email protected]>
  2022-08-29 16:54                                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Robert Haas @ 2022-08-29 16:54 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers

On Wed, Aug 10, 2022 at 4:37 AM Kyotaro Horiguchi
<[email protected]> wrote:
> So, it seems that the *standby* received the inconsistent WAL stream
> (aborted-contrecord not followed by a overwriting-missing-contrecord)
> from the primary.  Thus the inconsistency happened on the primary, not
> on the standby.
>
> So... I'm still failing to draw the big picutre of what is happening
> here.

For the benefit of anyone who may be looking at this thread in the
archive later, I believe this commit will have fixed this issue:


-- 
Robert Haas
EDB: http://www.enterprisedb.com





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-10 08:37                               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-29 16:54                                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
@ 2022-08-29 16:54                                   ` Robert Haas <[email protected]>
  2022-08-31 13:15                                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Robert Haas @ 2022-08-29 16:54 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers

On Mon, Aug 29, 2022 at 12:54 PM Robert Haas <[email protected]> wrote:
> On Wed, Aug 10, 2022 at 4:37 AM Kyotaro Horiguchi
> <[email protected]> wrote:
> > So, it seems that the *standby* received the inconsistent WAL stream
> > (aborted-contrecord not followed by a overwriting-missing-contrecord)
> > from the primary.  Thus the inconsistency happened on the primary, not
> > on the standby.
> >
> > So... I'm still failing to draw the big picutre of what is happening
> > here.
>
> For the benefit of anyone who may be looking at this thread in the
> archive later, I believe this commit will have fixed this issue:

Err, this commit here, I mean:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6672d7913929b82ec723a54381773d9cdc20f...

-- 
Robert Haas
EDB: http://www.enterprisedb.com





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
  2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
  2022-08-10 08:37                               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
  2022-08-29 16:54                                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
  2022-08-29 16:54                                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
@ 2022-08-31 13:15                                     ` Imseih (AWS), Sami <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Imseih (AWS), Sami @ 2022-08-31 13:15 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers

>    > For the benefit of anyone who may be looking at this thread in the
>    > archive later, I believe this commit will have fixed this issue:

I can also confirm that indeed the commit https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6672d79
does fix this issue.

Thanks!

--
Sami Imseih
Amazon Web Services (AWS)





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
@ 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; 34+ 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] 34+ messages in thread

* Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
  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; 34+ 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] 34+ messages in thread

* [PATCH v19 6/8] Row pattern recognition patch (docs).
@ 2024-05-14 23:26 Tatsuo Ishii <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Tatsuo Ishii @ 2024-05-14 23:26 UTC (permalink / raw)

---
 doc/src/sgml/advanced.sgml   | 82 ++++++++++++++++++++++++++++++++++++
 doc/src/sgml/func.sgml       | 54 ++++++++++++++++++++++++
 doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++-
 3 files changed, 172 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 755c9f1485..b0b1d1c51e 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -537,6 +537,88 @@ WHERE pos &lt; 3;
     <literal>rank</literal> less than 3.
    </para>
 
+   <para>
+    Row pattern common syntax can be used to perform row pattern recognition
+    in a query. The row pattern common syntax includes two sub
+    clauses: <literal>DEFINE</literal>
+    and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines
+    definition variables along with an expression. The expression must be a
+    logical expression, which means it must
+    return <literal>TRUE</literal>, <literal>FALSE</literal>
+    or <literal>NULL</literal>. The expression may comprise column references
+    and functions. Window functions, aggregate functions and subqueries are
+    not allowed. An example of <literal>DEFINE</literal> is as follows.
+
+<programlisting>
+DEFINE
+ LOWPRICE AS price &lt;= 100,
+ UP AS price &gt; PREV(price),
+ DOWN AS price &lt; PREV(price)
+</programlisting>
+
+    Note that <function>PREV</function> returns the price column in the
+    previous row if it's called in a context of row pattern recognition. Thus in
+    the second line the definition variable "UP" is <literal>TRUE</literal>
+    when the price column in the current row is greater than the price column
+    in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when
+    the price column in the current row is lower than the price column in the
+    previous row.
+   </para>
+   <para>
+    Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be
+    used. <literal>PATTERN</literal> defines a sequence of rows that satisfies
+    certain conditions.  For example following <literal>PATTERN</literal>
+    defines that a row starts with the condition "LOWPRICE", then one or more
+    rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that
+    "+" means one or more matches. Also you can use "*", which means zero or
+    more matches. If a sequence of rows which satisfies the PATTERN is found,
+    in the starting row of the sequence of rows all window functions and
+    aggregates are shown in the target list. Note that aggregations only look
+    into the matched rows, rather than whole frame. On the second or
+    subsequent rows all window functions are NULL. Aggregates are NULL or 0
+    (count case) depending on its aggregation definition. For rows that do not
+    match on the PATTERN, all window functions and aggregates are shown AS
+    NULL too, except count showing 0. This is because the rows do not match,
+    thus they are in an empty frame. Example of a <literal>SELECT</literal>
+    using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause
+    is as follows.
+
+<programlisting>
+SELECT company, tdate, price,
+ first_value(price) OVER w,
+ max(price) OVER w,
+ count(price) OVER w
+FROM stock
+ WINDOW w AS (
+ PARTITION BY company
+ ORDER BY tdate
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ AFTER MATCH SKIP PAST LAST ROW
+ INITIAL
+ PATTERN (LOWPRICE UP+ DOWN+)
+ DEFINE
+  LOWPRICE AS price &lt;= 100,
+  UP AS price &gt; PREV(price),
+  DOWN AS price &lt; PREV(price)
+);
+</programlisting>
+<screen>
+ company  |   tdate    | price | first_value | max | count 
+----------+------------+-------+-------------+-----+-------
+ company1 | 2023-07-01 |   100 |         100 | 200 |     4
+ company1 | 2023-07-02 |   200 |             |     |     0
+ company1 | 2023-07-03 |   150 |             |     |     0
+ company1 | 2023-07-04 |   140 |             |     |     0
+ company1 | 2023-07-05 |   150 |             |     |     0
+ company1 | 2023-07-06 |    90 |          90 | 130 |     4
+ company1 | 2023-07-07 |   110 |             |     |     0
+ company1 | 2023-07-08 |   130 |             |     |     0
+ company1 | 2023-07-09 |   120 |             |     |     0
+ company1 | 2023-07-10 |   130 |             |     |     0
+(10 rows)
+</screen>
+   </para>
+
    <para>
     When a query involves multiple window functions, it is possible to write
     out each one with a separate <literal>OVER</literal> clause, but this is
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 17c44bc338..8dbab31300 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23124,6 +23124,7 @@ SELECT count(*) FROM sometable;
         returns <literal>NULL</literal> if there is no such row.
        </para></entry>
       </row>
+
      </tbody>
     </tgroup>
    </table>
@@ -23163,6 +23164,59 @@ SELECT count(*) FROM sometable;
    Other frame specifications can be used to obtain other effects.
   </para>
 
+  <para>
+   Row pattern recognition navigation functions are listed in
+   <xref linkend="functions-rpr-navigation-table"/>.  These functions
+   can be used to describe DEFINE clause of Row pattern recognition.
+  </para>
+
+   <table id="functions-rpr-navigation-table">
+    <title>Row Pattern Navigation Functions</title>
+    <tgroup cols="1">
+     <thead>
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        Function
+       </para>
+       <para>
+        Description
+       </para></entry>
+      </row>
+     </thead>
+
+     <tbody>
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>prev</primary>
+        </indexterm>
+        <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> )
+        <returnvalue>anyelement</returnvalue>
+       </para>
+       <para>
+        Returns the column value at the previous row;
+        returns NULL if there is no previous row in the window frame.
+       </para></entry>
+      </row>
+
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>next</primary>
+        </indexterm>
+        <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> )
+        <returnvalue>anyelement</returnvalue>
+       </para>
+       <para>
+        Returns the column value at the next row;
+        returns NULL if there is no next row in the window frame.
+       </para></entry>
+      </row>
+
+     </tbody>
+    </tgroup>
+   </table>
+
   <note>
    <para>
     The SQL standard defines a <literal>RESPECT NULLS</literal> or
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 066aed44e6..8f18718d58 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl
     The <replaceable class="parameter">frame_clause</replaceable> can be one of
 
 <synopsis>
-{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
-{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
+{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
+{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
 </synopsis>
 
     where <replaceable>frame_start</replaceable>
@@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS
     a given peer group will be in the frame or excluded from it.
    </para>
 
+   <para>
+    The
+    optional <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+    defines the <firstterm>row pattern recognition condition</firstterm> for
+    this
+    window. <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+    includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST
+    ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls
+    how to proceed to next row position after a match
+    found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the
+    default) next row position is next to the last row of previous match. On
+    the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next
+    row position is always next to the last row of previous
+    match. <literal>DEFINE</literal> defines definition variables along with a
+    boolean expression. <literal>PATTERN</literal> defines a sequence of rows
+    that satisfies certain conditions using variables defined
+    in <literal>DEFINE</literal> clause. If the variable is not defined in
+    the <literal>DEFINE</literal> clause, it is implicitly assumed
+    following is defined in the <literal>DEFINE</literal> clause.
+
+<synopsis>
+<literal>variable_name</literal> AS TRUE
+</synopsis>
+
+    Note that the maximu number of variables defined
+    in <literal>DEFINE</literal> clause is 26.
+
+<synopsis>
+[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
+PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...]
+DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...]
+</synopsis>
+   </para>
+
    <para>
     The purpose of a <literal>WINDOW</literal> clause is to specify the
     behavior of <firstterm>window functions</firstterm> appearing in the query's
-- 
2.25.1


----Next_Part(Wed_May_15_09_02_03_2024_008)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v19-0007-Row-pattern-recognition-patch-tests.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread


end of thread, other threads:[~2024-05-14 23:26 UTC | newest]

Thread overview: 34+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-06-18 12:34 [PATCH 3/8] Add rewrite rules and tupdesc flags Ildus Kurbangaliev <[email protected]>
2022-02-24 08:27 Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-02-25 21:28 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-03-07 20:34   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-03-23 17:24     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion [email protected] <[email protected]>
2022-05-26 19:57     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-05-27 01:59       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-05-27 02:01       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-05-27 06:37         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-05-27 19:01           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-06-02 18:28             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Hsu, John <[email protected]>
2022-06-20 07:13             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
2022-06-20 11:28               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-06-20 15:57                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
2022-06-21 01:35                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-06-22 01:41                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
2022-06-22 14:38                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
2022-06-24 16:17                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-06-27 06:02                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Michael Paquier <[email protected]>
2022-06-28 00:46                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-06-28 07:09                     ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-06-29 13:47                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-07-28 08:02                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion [email protected] <[email protected]>
2022-07-29 15:07                       ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-08-05 21:28                         ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-08-08 04:06                           ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-08-08 15:51                             ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Imseih (AWS), Sami <[email protected]>
2022-08-10 08:37                               ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Kyotaro Horiguchi <[email protected]>
2022-08-29 16:54                                 ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
2022-08-29 16:54                                   ` Re: [BUG] Panic due to incorrect missingContrecPtr after promotion Robert Haas <[email protected]>
2022-08-31 13:15                                     ` Re: [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]>
2024-05-14 23:26 [PATCH v19 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[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