public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 3/8] Add rewrite rules and tupdesc flags
11+ messages / 5 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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]>
  0 siblings, 0 replies; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread


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

Thread overview: 11+ 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]>
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