public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH v7 04/16] heap_page_prune sets all_visible and visibility_cutoff_xid
3+ messages / 3 participants
[nested] [flat]

* [PATCH v7 04/16] heap_page_prune sets all_visible and visibility_cutoff_xid
@ 2024-03-25 22:31  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 3+ messages in thread

From: Melanie Plageman @ 2024-03-25 22:31 UTC (permalink / raw)

In order to combine the prune and freeze records, we must know if the
page is eligible to be opportunistically frozen before finishing
pruning. Save all_visible in the PruneResult and set it to false when we
see non-removable tuples which are not visible to everyone.

We will also need to ensure that the snapshotConflictHorizon for the combined
prune + freeze record is the more conservative of that calculated for each of
pruning and freezing. Calculate the visibility_cutoff_xid for the purposes of
freezing -- the newest xmin on the page -- in heap_page_prune() and save it in
PruneResult.visibility_cutoff_xid.

Note that these are only needed by vacuum callers of heap_page_prune(),
so don't update them for on-access pruning.
---
 src/backend/access/heap/pruneheap.c  | 131 +++++++++++++++++++++++++--
 src/backend/access/heap/vacuumlazy.c | 113 +++++------------------
 src/include/access/heapam.h          |  21 +++++
 3 files changed, 169 insertions(+), 96 deletions(-)

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index ca4301bb8a9..5776ae84f4d 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -63,8 +63,10 @@ static int	heap_prune_chain(Buffer buffer,
 static void heap_prune_record_prunable(PruneState *prstate, TransactionId xid);
 static void heap_prune_record_redirect(PruneState *prstate,
 									   OffsetNumber offnum, OffsetNumber rdoffnum);
-static void heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum);
-static void heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum);
+static void heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum,
+								   PruneResult *presult);
+static void heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum,
+											 PruneResult *presult);
 static void heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum);
 static void page_verify_redirects(Page page);
 
@@ -249,6 +251,14 @@ heap_page_prune(Relation relation, Buffer buffer,
 	presult->ndeleted = 0;
 	presult->nnewlpdead = 0;
 
+	/*
+	 * Keep track of whether or not the page is all_visible in case the caller
+	 * wants to use this information to update the VM.
+	 */
+	presult->all_visible = true;
+	/* for recovery conflicts */
+	presult->visibility_cutoff_xid = InvalidTransactionId;
+
 	maxoff = PageGetMaxOffsetNumber(page);
 	tup.t_tableOid = RelationGetRelid(relation);
 
@@ -300,8 +310,101 @@ heap_page_prune(Relation relation, Buffer buffer,
 
 		presult->htsv[offnum] = heap_prune_satisfies_vacuum(&prstate, &tup,
 															buffer);
+
+		if (reason == PRUNE_ON_ACCESS)
+			continue;
+
+		switch (presult->htsv[offnum])
+		{
+			case HEAPTUPLE_DEAD:
+
+				/*
+				 * Deliberately delay unsetting all_visible until later during
+				 * pruning. Removable dead tuples shouldn't preclude freezing
+				 * the page. After finishing this first pass of tuple
+				 * visibility checks, initialize all_visible_except_removable
+				 * with the current value of all_visible to indicate whether
+				 * or not the page is all visible except for dead tuples. This
+				 * will allow us to attempt to freeze the page after pruning.
+				 * Later during pruning, if we encounter an LP_DEAD item or
+				 * are setting an item LP_DEAD, we will unset all_visible. As
+				 * long as we unset it before updating the visibility map,
+				 * this will be correct.
+				 */
+				break;
+			case HEAPTUPLE_LIVE:
+
+				/*
+				 * Is the tuple definitely visible to all transactions?
+				 *
+				 * NB: Like with per-tuple hint bits, we can't set the
+				 * PD_ALL_VISIBLE flag if the inserter committed
+				 * asynchronously. See SetHintBits for more info. Check that
+				 * the tuple is hinted xmin-committed because of that.
+				 */
+				if (presult->all_visible)
+				{
+					TransactionId xmin;
+
+					if (!HeapTupleHeaderXminCommitted(htup))
+					{
+						presult->all_visible = false;
+						break;
+					}
+
+					/*
+					 * The inserter definitely committed. But is it old enough
+					 * that everyone sees it as committed? A
+					 * FrozenTransactionId is seen as committed to everyone.
+					 * Otherwise, we check if there is a snapshot that
+					 * considers this xid to still be running, and if so, we
+					 * don't consider the page all-visible.
+					 */
+					xmin = HeapTupleHeaderGetXmin(htup);
+					if (xmin != FrozenTransactionId &&
+						!GlobalVisTestIsRemovableXid(vistest, xmin))
+					{
+						presult->all_visible = false;
+						break;
+					}
+
+					/* Track newest xmin on page. */
+					if (TransactionIdFollows(xmin, presult->visibility_cutoff_xid) &&
+						TransactionIdIsNormal(xmin))
+						presult->visibility_cutoff_xid = xmin;
+				}
+				break;
+			case HEAPTUPLE_RECENTLY_DEAD:
+				presult->all_visible = false;
+				break;
+			case HEAPTUPLE_INSERT_IN_PROGRESS:
+				presult->all_visible = false;
+				break;
+			case HEAPTUPLE_DELETE_IN_PROGRESS:
+				/* This is an expected case during concurrent vacuum */
+				presult->all_visible = false;
+				break;
+			default:
+				elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result");
+				break;
+		}
 	}
 
+	/*
+	 * For vacuum, if the whole page will become frozen, we consider
+	 * opportunistically freezing tuples. Dead tuples which will be removed by
+	 * the end of vacuuming should not preclude us from opportunistically
+	 * freezing. We will not be able to freeze the whole page if there are
+	 * tuples present which are not visible to everyone or if there are dead
+	 * tuples which are not yet removable. We need all_visible to be false if
+	 * LP_DEAD tuples remain after pruning so that we do not incorrectly
+	 * update the visibility map or page hint bit. So, we will update
+	 * presult->all_visible to reflect the presence of LP_DEAD items while
+	 * pruning and keep all_visible_except_removable to permit freezing if the
+	 * whole page will eventually become all visible after removing tuples.
+	 */
+	presult->all_visible_except_removable = presult->all_visible;
+
 	/* Scan the page */
 	for (offnum = FirstOffsetNumber;
 		 offnum <= maxoff;
@@ -569,10 +672,14 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
 			/*
 			 * If the caller set mark_unused_now true, we can set dead line
 			 * pointers LP_UNUSED now. We don't increment ndeleted here since
-			 * the LP was already marked dead.
+			 * the LP was already marked dead. If it will not be marked
+			 * LP_UNUSED, it will remain LP_DEAD, making the page not
+			 * all_visible.
 			 */
 			if (unlikely(prstate->mark_unused_now))
 				heap_prune_record_unused(prstate, offnum);
+			else
+				presult->all_visible = false;
 
 			break;
 		}
@@ -709,7 +816,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
 		 * redirect the root to the correct chain member.
 		 */
 		if (i >= nchain)
-			heap_prune_record_dead_or_unused(prstate, rootoffnum);
+			heap_prune_record_dead_or_unused(prstate, rootoffnum, presult);
 		else
 			heap_prune_record_redirect(prstate, rootoffnum, chainitems[i]);
 	}
@@ -722,7 +829,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
 		 * redirect item.  We can clean up by setting the redirect item to
 		 * DEAD state or LP_UNUSED if the caller indicated.
 		 */
-		heap_prune_record_dead_or_unused(prstate, rootoffnum);
+		heap_prune_record_dead_or_unused(prstate, rootoffnum, presult);
 	}
 
 	return ndeleted;
@@ -759,13 +866,20 @@ heap_prune_record_redirect(PruneState *prstate,
 
 /* Record line pointer to be marked dead */
 static void
-heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum)
+heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum,
+					   PruneResult *presult)
 {
 	Assert(prstate->ndead < MaxHeapTuplesPerPage);
 	prstate->nowdead[prstate->ndead] = offnum;
 	prstate->ndead++;
 	Assert(!prstate->marked[offnum]);
 	prstate->marked[offnum] = true;
+
+	/*
+	 * Setting the line pointer LP_DEAD means the page will definitely not be
+	 * all_visible.
+	 */
+	presult->all_visible = false;
 }
 
 /*
@@ -775,7 +889,8 @@ heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum)
  * pointers LP_DEAD if mark_unused_now is true.
  */
 static void
-heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum)
+heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum,
+								 PruneResult *presult)
 {
 	/*
 	 * If the caller set mark_unused_now to true, we can remove dead tuples
@@ -786,7 +901,7 @@ heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum)
 	if (unlikely(prstate->mark_unused_now))
 		heap_prune_record_unused(prstate, offnum);
 	else
-		heap_prune_record_dead(prstate, offnum);
+		heap_prune_record_dead(prstate, offnum, presult);
 }
 
 /* Record line pointer to be marked unused */
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index a7451743e25..17fb0b4f7b7 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1422,9 +1422,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
-	bool		all_visible,
-				all_frozen;
-	TransactionId visibility_cutoff_xid;
+	bool		all_frozen;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 	HeapTupleFreeze frozen[MaxHeapTuplesPerPage];
@@ -1465,17 +1463,16 @@ lazy_scan_prune(LVRelState *vacrel,
 					&presult, PRUNE_VACUUM_SCAN, &vacrel->offnum);
 
 	/*
-	 * We will update the VM after collecting LP_DEAD items and freezing
-	 * tuples. Keep track of whether or not the page is all_visible and
-	 * all_frozen and use this information to update the VM. all_visible
-	 * implies 0 lpdead_items, but don't trust all_frozen result unless
-	 * all_visible is also set to true.
+	 * Now scan the page to collect LP_DEAD items and check for tuples
+	 * requiring freezing among remaining tuples with storage. We will update
+	 * the VM after collecting LP_DEAD items and freezing tuples. Pruning will
+	 * have determined whether or not the page is all_visible. Keep track of
+	 * whether or not the page is all_frozen and use this information to
+	 * update the VM. all_visible implies lpdead_items == 0, but don't trust
+	 * all_frozen result unless all_visible is also set to true.
 	 *
-	 * Also keep track of the visibility cutoff xid for recovery conflicts.
 	 */
-	all_visible = true;
 	all_frozen = true;
-	visibility_cutoff_xid = InvalidTransactionId;
 
 	/*
 	 * Now scan the page to collect LP_DEAD items and update the variables set
@@ -1516,11 +1513,6 @@ lazy_scan_prune(LVRelState *vacrel,
 			 * will only happen every other VACUUM, at most.  Besides, VACUUM
 			 * must treat hastup/nonempty_pages as provisional no matter how
 			 * LP_DEAD items are handled (handled here, or handled later on).
-			 *
-			 * Also deliberately delay unsetting all_visible until just before
-			 * we return to lazy_scan_heap caller, as explained in full below.
-			 * (This is another case where it's useful to anticipate that any
-			 * LP_DEAD items will become LP_UNUSED during the ongoing VACUUM.)
 			 */
 			deadoffsets[lpdead_items++] = offnum;
 			continue;
@@ -1558,46 +1550,6 @@ lazy_scan_prune(LVRelState *vacrel,
 				 * what acquire_sample_rows() does.
 				 */
 				live_tuples++;
-
-				/*
-				 * Is the tuple definitely visible to all transactions?
-				 *
-				 * NB: Like with per-tuple hint bits, we can't set the
-				 * PD_ALL_VISIBLE flag if the inserter committed
-				 * asynchronously. See SetHintBits for more info. Check that
-				 * the tuple is hinted xmin-committed because of that.
-				 */
-				if (all_visible)
-				{
-					TransactionId xmin;
-
-					if (!HeapTupleHeaderXminCommitted(htup))
-					{
-						all_visible = false;
-						break;
-					}
-
-					/*
-					 * The inserter definitely committed. But is it old enough
-					 * that everyone sees it as committed? A
-					 * FrozenTransactionId is seen as committed to everyone.
-					 * Otherwise, we check if there is a snapshot that
-					 * considers this xid to still be running, and if so, we
-					 * don't consider the page all-visible.
-					 */
-					xmin = HeapTupleHeaderGetXmin(htup);
-					if (xmin != FrozenTransactionId &&
-						!GlobalVisTestIsRemovableXid(vacrel->vistest, xmin))
-					{
-						all_visible = false;
-						break;
-					}
-
-					/* Track newest xmin on page. */
-					if (TransactionIdFollows(xmin, visibility_cutoff_xid) &&
-						TransactionIdIsNormal(xmin))
-						visibility_cutoff_xid = xmin;
-				}
 				break;
 			case HEAPTUPLE_RECENTLY_DEAD:
 
@@ -1607,7 +1559,6 @@ lazy_scan_prune(LVRelState *vacrel,
 				 * pruning.)
 				 */
 				recently_dead_tuples++;
-				all_visible = false;
 				break;
 			case HEAPTUPLE_INSERT_IN_PROGRESS:
 
@@ -1618,16 +1569,13 @@ lazy_scan_prune(LVRelState *vacrel,
 				 * results.  This assumption is a bit shaky, but it is what
 				 * acquire_sample_rows() does, so be consistent.
 				 */
-				all_visible = false;
 				break;
 			case HEAPTUPLE_DELETE_IN_PROGRESS:
-				/* This is an expected case during concurrent vacuum */
-				all_visible = false;
 
 				/*
-				 * Count such rows as live.  As above, we assume the deleting
-				 * transaction will commit and update the counters after we
-				 * report.
+				 * This an expected case during concurrent vacuum. Count such
+				 * rows as live.  As above, we assume the deleting transaction
+				 * will commit and update the counters after we report.
 				 */
 				live_tuples++;
 				break;
@@ -1670,7 +1618,7 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
 	if (pagefrz.freeze_required || tuples_frozen == 0 ||
-		(all_visible && all_frozen &&
+		(presult.all_visible_except_removable && all_frozen &&
 		 fpi_before != pgWalUsage.wal_fpi))
 	{
 		/*
@@ -1708,11 +1656,11 @@ lazy_scan_prune(LVRelState *vacrel,
 			 * once we're done with it.  Otherwise we generate a conservative
 			 * cutoff by stepping back from OldestXmin.
 			 */
-			if (all_visible && all_frozen)
+			if (presult.all_visible_except_removable && all_frozen)
 			{
 				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = visibility_cutoff_xid;
-				visibility_cutoff_xid = InvalidTransactionId;
+				snapshotConflictHorizon = presult.visibility_cutoff_xid;
+				presult.visibility_cutoff_xid = InvalidTransactionId;
 			}
 			else
 			{
@@ -1748,17 +1696,19 @@ lazy_scan_prune(LVRelState *vacrel,
 	 */
 #ifdef USE_ASSERT_CHECKING
 	/* Note that all_frozen value does not matter when !all_visible */
-	if (all_visible && lpdead_items == 0)
+	if (presult.all_visible)
 	{
 		TransactionId debug_cutoff;
 		bool		debug_all_frozen;
 
+		Assert(lpdead_items == 0);
+
 		if (!heap_page_is_all_visible(vacrel, buf,
 									  &debug_cutoff, &debug_all_frozen))
 			Assert(false);
 
 		Assert(!TransactionIdIsValid(debug_cutoff) ||
-			   debug_cutoff == visibility_cutoff_xid);
+			   debug_cutoff == presult.visibility_cutoff_xid);
 	}
 #endif
 
@@ -1783,19 +1733,6 @@ lazy_scan_prune(LVRelState *vacrel,
 		Assert(dead_items->num_items <= dead_items->max_items);
 		pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
 									 dead_items->num_items);
-
-		/*
-		 * It was convenient to ignore LP_DEAD items in all_visible earlier on
-		 * to make the choice of whether or not to freeze the page unaffected
-		 * by the short-term presence of LP_DEAD items.  These LP_DEAD items
-		 * were effectively assumed to be LP_UNUSED items in the making.  It
-		 * doesn't matter which heap pass (initial pass or final pass) ends up
-		 * setting the page all-frozen, as long as the ongoing VACUUM does it.
-		 *
-		 * Now that freezing has been finalized, unset all_visible.  It needs
-		 * to reflect the present state of things, as expected by our caller.
-		 */
-		all_visible = false;
 	}
 
 	/* Finally, add page-local counts to whole-VACUUM counts */
@@ -1812,20 +1749,20 @@ lazy_scan_prune(LVRelState *vacrel,
 	/* Did we find LP_DEAD items? */
 	*has_lpdead_items = (lpdead_items > 0);
 
-	Assert(!all_visible || !(*has_lpdead_items));
+	Assert(!presult.all_visible || !(*has_lpdead_items));
 
 	/*
 	 * Handle setting visibility map bit based on information from the VM (as
 	 * of last heap_vac_scan_next_block() call), and from all_visible and
 	 * all_frozen variables
 	 */
-	if (!all_visible_according_to_vm && all_visible)
+	if (!all_visible_according_to_vm && presult.all_visible)
 	{
 		uint8		flags = VISIBILITYMAP_ALL_VISIBLE;
 
 		if (all_frozen)
 		{
-			Assert(!TransactionIdIsValid(visibility_cutoff_xid));
+			Assert(!TransactionIdIsValid(presult.visibility_cutoff_xid));
 			flags |= VISIBILITYMAP_ALL_FROZEN;
 		}
 
@@ -1845,7 +1782,7 @@ lazy_scan_prune(LVRelState *vacrel,
 		PageSetAllVisible(page);
 		MarkBufferDirty(buf);
 		visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr,
-						  vmbuffer, visibility_cutoff_xid,
+						  vmbuffer, presult.visibility_cutoff_xid,
 						  flags);
 	}
 
@@ -1893,7 +1830,7 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * it as all-frozen.  Note that all_frozen is only valid if all_visible is
 	 * true, so we must check both all_visible and all_frozen.
 	 */
-	else if (all_visible_according_to_vm && all_visible &&
+	else if (all_visible_according_to_vm && presult.all_visible &&
 			 all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
 	{
 		/*
@@ -1914,7 +1851,7 @@ lazy_scan_prune(LVRelState *vacrel,
 		 * since a snapshotConflictHorizon sufficient to make everything safe
 		 * for REDO was logged when the page's tuples were frozen.
 		 */
-		Assert(!TransactionIdIsValid(visibility_cutoff_xid));
+		Assert(!TransactionIdIsValid(presult.visibility_cutoff_xid));
 		visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr,
 						  vmbuffer, InvalidTransactionId,
 						  VISIBILITYMAP_ALL_VISIBLE |
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 368c570a0f4..8d0dd40ba6d 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -199,6 +199,27 @@ typedef struct PruneResult
 	int			ndeleted;		/* Number of tuples deleted from the page */
 	int			nnewlpdead;		/* Number of newly LP_DEAD items */
 
+	/*
+	 * The rest of the fields in PruneResult are only guaranteed to be
+	 * initialized if heap_page_prune is passed PruneReason VACUUM_SCAN.
+	 */
+
+	/*
+	 * Whether or not the page is truly all-visible after pruning. If there
+	 * are LP_DEAD items on the page which cannot be removed until vacuum's
+	 * second pass, this will be false.
+	 */
+	bool		all_visible;
+
+	/*
+	 * Whether or not the page is all-visible except for tuples which will be
+	 * removed during vacuum's second pass. This is used by VACUUM to
+	 * determine whether or not to consider opportunistically freezing the
+	 * page.
+	 */
+	bool		all_visible_except_removable;
+	TransactionId visibility_cutoff_xid;	/* Newest xmin on the page */
+
 	/*
 	 * Tuple visibility is only computed once for each tuple, for correctness
 	 * and efficiency reasons; see comment in heap_page_prune() for details.
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0005-Add-reference-to-VacuumCutoffs-in-HeapPageFreeze.patch"



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

* Re: Virtual generated columns
@ 2025-02-09 11:01  Zhang Mingli <[email protected]>
  0 siblings, 1 reply; 3+ messages in thread

From: Zhang Mingli @ 2025-02-09 11:01 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; Dean Rasheed <[email protected]>; Alexander Lakhin <[email protected]>; +Cc: pgsql-hackers; jian he <[email protected]>

On Feb 9, 2025 at 16:00 +0800, Alexander Lakhin <[email protected]>, wrote:
>
> Please look at a planner error with a virtual generated column triggered
> by the following script:
> CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a * 1));
>
> SELECT SUM(CASE WHEN t.b = 1 THEN 1 ELSE 1 END) OVER (PARTITION BY t.a)
> FROM t AS t1 LEFT JOIN T ON true;
>
> ERROR:  XX000: wrong varnullingrels (b) (expected (b 3)) for Var 2/1
> LOCATION:  search_indexed_tlist_for_var, setrefs.c:2901
Hi,

I've been investigating for a while and here are my findings.

During the parse stage, we set the Var->varnullingrels in the parse_analyze_fixedparams function.
Later, when rewriting the parse tree in pg_rewrite_query() to expand virtual columns, we replace the expression column b with a new Var that includes a, since b is defined as a * 1.
Unfortunately, we overlooked updating the Var->varnullingrels at this point.
As a result, when we enter search_indexed_tlist_for_var, it leads to a failure.
While we do have another target entry with the correct varnullingrels, the expression involving the virtual column generates another column reference, which causes the error.
Currently, I don't have a solid fix.
One potential solution is to correct the Vars at or after the rewrite stage by traversing the parse tree again using markNullableIfNeeded.
However, this approach may require exposing the ParseState, which doesn't seem ideal.
It appears that the virtual column generation function during the rewrite stage does not account for the Var field settings, leading to the errors we are encountering.

--
Zhang Mingli
HashData


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

* Re: Virtual generated columns
@ 2025-02-10 03:54  Richard Guo <[email protected]>
  parent: Zhang Mingli <[email protected]>
  0 siblings, 0 replies; 3+ messages in thread

From: Richard Guo @ 2025-02-10 03:54 UTC (permalink / raw)
  To: Zhang Mingli <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Dean Rasheed <[email protected]>; Alexander Lakhin <[email protected]>; pgsql-hackers; jian he <[email protected]>

On Sun, Feb 9, 2025 at 7:02 PM Zhang Mingli <[email protected]> wrote:
> On Feb 9, 2025 at 16:00 +0800, Alexander Lakhin <[email protected]>, wrote:
> Please look at a planner error with a virtual generated column triggered
> by the following script:
> CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a * 1));
>
> SELECT SUM(CASE WHEN t.b = 1 THEN 1 ELSE 1 END) OVER (PARTITION BY t.a)
> FROM t AS t1 LEFT JOIN T ON true;
>
> ERROR:  XX000: wrong varnullingrels (b) (expected (b 3)) for Var 2/1
> LOCATION:  search_indexed_tlist_for_var, setrefs.c:2901

> During the parse stage, we set the Var->varnullingrels in the parse_analyze_fixedparams function.
> Later, when rewriting the parse tree in pg_rewrite_query() to expand virtual columns, we replace the expression column b with a new Var that includes a, since b is defined as a * 1.
> Unfortunately, we overlooked updating the Var->varnullingrels at this point.
> As a result, when we enter search_indexed_tlist_for_var, it leads to a failure.
> While we do have another target entry with the correct varnullingrels, the expression involving the virtual column generates another column reference, which causes the error.
> Currently, I don't have a solid fix.
> One potential solution is to correct the Vars at or after the rewrite stage by traversing the parse tree again using markNullableIfNeeded.
> However, this approach may require exposing the ParseState, which doesn't seem ideal.
> It appears that the virtual column generation function during the rewrite stage does not account for the Var field settings, leading to the errors we are encountering.

Hmm, would it be possible to propagate any varnullingrels into the
replacement expression in ReplaceVarsFromTargetList_callback()?

BTW, I was curious about what happens if the replacement expression is
constant, so I tried running the query below.

CREATE TABLE t (a int, b int GENERATED ALWAYS AS (1 + 1));
INSERT INTO t VALUES (1);
INSERT INTO t VALUES (2);

# SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;
 a | b
---+---
   | 2
   | 2
(2 rows)

Is this the expected behavior?  I was expecting that t2.b should be
all NULLs.

Thanks
Richard






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


end of thread, other threads:[~2025-02-10 03:54 UTC | newest]

Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-03-25 22:31 [PATCH v7 04/16] heap_page_prune sets all_visible and visibility_cutoff_xid Melanie Plageman <[email protected]>
2025-02-09 11:01 Re: Virtual generated columns Zhang Mingli <[email protected]>
2025-02-10 03:54 ` Re: Virtual generated columns Richard Guo <[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