stray references to SubscriptRef type

Started by Peter Eisentraut4 months ago2 messages
#1Peter Eisentraut
peter@eisentraut.org
1 attachment(s)

There are several mentions in code comments of a SubscriptRef type, but
that type does not exist and AFAICT never existed. (Perhaps it existed
in in-development code at some point.)

I think in several of those cases this clearly meant the type
SubscriptingRef instead. But the mentions in
src/include/nodes/subscripting.h probably meant the various sbs_* functions.

If so, it would also be useful perhaps to document the leakproof
expectations of the other sbs_* functions (sbs_fetch_old and
sbs_check_subscripts).

See attached patch for my solution so far.

Attachments:

0001-Fix-stray-references-to-SubscriptRef.patchtext/plain; charset=UTF-8; name=0001-Fix-stray-references-to-SubscriptRef.patchDownload
From 7d0a0967429ffb28e6e46fc7fa5f2af9e19d5d9a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 9 Sep 2025 16:16:38 +0200
Subject: [PATCH] Fix stray references to SubscriptRef

This type never existed.
---
 contrib/hstore/hstore_subs.c      |  2 +-
 src/backend/utils/adt/arraysubs.c |  2 +-
 src/backend/utils/adt/jsonbsubs.c |  2 +-
 src/include/nodes/subscripting.h  | 14 +++++++-------
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/hstore/hstore_subs.c b/contrib/hstore/hstore_subs.c
index 3d03f66fa0d..1bae69e4e2c 100644
--- a/contrib/hstore/hstore_subs.c
+++ b/contrib/hstore/hstore_subs.c
@@ -74,7 +74,7 @@ hstore_subscript_transform(SubscriptingRef *sbsref,
 				 errmsg("hstore subscript must have type text"),
 				 parser_errposition(pstate, exprLocation(ai->uidx))));
 
-	/* ... and store the transformed subscript into the SubscriptRef node */
+	/* ... and store the transformed subscript into the SubscriptingRef node */
 	sbsref->refupperindexpr = list_make1(subexpr);
 	sbsref->reflowerindexpr = NIL;
 
diff --git a/src/backend/utils/adt/arraysubs.c b/src/backend/utils/adt/arraysubs.c
index 2940fb8e8d7..b476fa586a9 100644
--- a/src/backend/utils/adt/arraysubs.c
+++ b/src/backend/utils/adt/arraysubs.c
@@ -140,7 +140,7 @@ array_subscript_transform(SubscriptingRef *sbsref,
 		upperIndexpr = lappend(upperIndexpr, subexpr);
 	}
 
-	/* ... and store the transformed lists into the SubscriptRef node */
+	/* ... and store the transformed lists into the SubscriptingRef node */
 	sbsref->refupperindexpr = upperIndexpr;
 	sbsref->reflowerindexpr = lowerIndexpr;
 
diff --git a/src/backend/utils/adt/jsonbsubs.c b/src/backend/utils/adt/jsonbsubs.c
index de64d498512..00820e5cdef 100644
--- a/src/backend/utils/adt/jsonbsubs.c
+++ b/src/backend/utils/adt/jsonbsubs.c
@@ -152,7 +152,7 @@ jsonb_subscript_transform(SubscriptingRef *sbsref,
 		upperIndexpr = lappend(upperIndexpr, subExpr);
 	}
 
-	/* store the transformed lists into the SubscriptRef node */
+	/* store the transformed lists into the SubscriptingRef node */
 	sbsref->refupperindexpr = upperIndexpr;
 	sbsref->reflowerindexpr = NIL;
 
diff --git a/src/include/nodes/subscripting.h b/src/include/nodes/subscripting.h
index 234e8ad8012..1f7f75d357f 100644
--- a/src/include/nodes/subscripting.h
+++ b/src/include/nodes/subscripting.h
@@ -35,14 +35,14 @@ struct SubscriptExecSteps;
  * several bool flags that specify properties of the subscripting actions
  * this data type can perform:
  *
- * fetch_strict indicates that a fetch SubscriptRef is strict, i.e., returns
+ * fetch_strict indicates that the sbs_fetch function is strict, i.e., returns
  * NULL if any input (either the container or any subscript) is NULL.
  *
- * fetch_leakproof indicates that a fetch SubscriptRef is leakproof, i.e.,
+ * fetch_leakproof indicates that the sbs_fetch function is leakproof, i.e.,
  * will not throw any data-value-dependent errors.  Typically this requires
  * silently returning NULL for invalid subscripts.
  *
- * store_leakproof similarly indicates whether an assignment SubscriptRef is
+ * store_leakproof similarly indicates whether the sbs_assign function is
  * leakproof.  (It is common to prefer throwing errors for invalid subscripts
  * in assignments; that's fine, but it makes the operation not leakproof.
  * In current usage there is no advantage in making assignments leakproof.)
@@ -51,7 +51,7 @@ struct SubscriptExecSteps;
  * undesirable, since for example a null subscript in an assignment would
  * cause the entire container to become NULL.
  *
- * Regardless of these flags, all SubscriptRefs are expected to be immutable,
+ * Regardless of these flags, both sbs_fetch and sbs_assign are expected to be immutable,
  * that is they must always give the same results for the same inputs.
  * They are expected to always be parallel-safe, as well.
  */
@@ -159,9 +159,9 @@ typedef struct SubscriptRoutines
 {
 	SubscriptTransform transform;	/* parse analysis function */
 	SubscriptExecSetup exec_setup;	/* expression compilation function */
-	bool		fetch_strict;	/* is fetch SubscriptRef strict? */
-	bool		fetch_leakproof;	/* is fetch SubscriptRef leakproof? */
-	bool		store_leakproof;	/* is assignment SubscriptRef leakproof? */
+	bool		fetch_strict;	/* is sbs_fetch strict? */
+	bool		fetch_leakproof;	/* is sbs_fetch leakproof? */
+	bool		store_leakproof;	/* is sbs_assign leakproof? */
 } SubscriptRoutines;
 
 #endif							/* SUBSCRIPTING_H */
-- 
2.51.0

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#1)
Re: stray references to SubscriptRef type

Peter Eisentraut <peter@eisentraut.org> writes:

There are several mentions in code comments of a SubscriptRef type, but
that type does not exist and AFAICT never existed. (Perhaps it existed
in in-development code at some point.)

Yeah, I think we went back and forth on the node name for awhile
before settling on SubscriptingRef. These comments evidently escaped
being updated.

I think in several of those cases this clearly meant the type
SubscriptingRef instead. But the mentions in
src/include/nodes/subscripting.h probably meant the various sbs_* functions.

I'm fairly certain that the SubscriptingRef node type was meant,
but I tend to agree that refocusing those comments on the method
functions would be an improvement. However, the way you've written it
here seems incorrect. What the comment is trying to say is that the
entire operation represented by the SubscriptingRef node is strict or
leakproof if those flags are set. Thus for example, fetch_leakproof
asserts that both sbs_fetch and sbs_check_subscripts (if used) are
leakproof. store_leakproof implicates sbs_check_subscripts,
sbs_assign, and sbs_fetch_old.

If so, it would also be useful perhaps to document the leakproof
expectations of the other sbs_* functions (sbs_fetch_old and
sbs_check_subscripts).

See above.

regards, tom lane