Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Sun, Nov 22, 2020 at 08:11:21PM +0900, Michael Paquier wrote:
> Okay, here you go with the attached.  If there are any other comments,
> please feel free.

Hearing nothing, applied this one after going through the ODBC driver
code again this morning.  Compatibility is exactly the same for
currtid2(), while currtid() is now gone.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Sat, Nov 21, 2020 at 09:39:28PM -0500, Tom Lane wrote:
> Michael Paquier  writes:
>> So, what you are basically saying is to switch currtid_byreloid() to
>> become a function local to tid.c.  And then have just
>> currtid_byrelname() and currtid_for_view() call that, right?
> 
> Yeah, that sounds about right.

Okay, here you go with the attached.  If there are any other comments,
please feel free.
--
Michael
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 92b19dba32..54b2eb7378 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -129,7 +129,6 @@ extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
    bool *all_dead, bool first_call);
 
 extern void heap_get_latest_tid(TableScanDesc scan, ItemPointer tid);
-extern void setLastTid(const ItemPointer tid);
 
 extern BulkInsertState GetBulkInsertState(void);
 extern void FreeBulkInsertState(BulkInsertState);
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index c6da0df868..5f33dc9db0 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*			mmddN */
-#define CATALOG_VERSION_NO	202011191
+#define CATALOG_VERSION_NO	202011221
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 33dacfd340..e7fbda9f81 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2549,9 +2549,6 @@
 { oid => '1292',
   proname => 'tideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'tid tid', prosrc => 'tideq' },
-{ oid => '1293', descr => 'latest tid of a tuple',
-  proname => 'currtid', provolatile => 'v', proparallel => 'u',
-  prorettype => 'tid', proargtypes => 'oid tid', prosrc => 'currtid_byreloid' },
 { oid => '1294', descr => 'latest tid of a tuple',
   proname => 'currtid2', provolatile => 'v', proparallel => 'u',
   prorettype => 'tid', proargtypes => 'text tid',
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 29e07b7228..e0f24283b8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -645,10 +645,7 @@ ExecInsert(ModifyTableState *mtstate,
 	}
 
 	if (canSetTag)
-	{
 		(estate->es_processed)++;
-		setLastTid(>tts_tid);
-	}
 
 	/*
 	 * If this insert is the result of a partition key update that moved the
diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 509a0fdffc..6f8b400e83 100644
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -47,6 +47,8 @@
 #define DELIM			','
 #define NTIDARGS		2
 
+static ItemPointer currtid_for_view(Relation viewrel, ItemPointer tid);
+
 /* 
  *		tidin
  * 
@@ -275,12 +277,44 @@ hashtidextended(PG_FUNCTION_ARGS)
  *	Maybe these implementations should be moved to another place
  */
 
-static ItemPointerData Current_last_tid = {{0, 0}, 0};
-
-void
-setLastTid(const ItemPointer tid)
+/*
+ * Utility wrapper for current CTID functions.
+ *		Returns the latest version of a tuple pointing at "tid" for
+ *		relation "rel".
+ */
+static ItemPointer
+currtid_internal(Relation rel, ItemPointer tid)
 {
-	Current_last_tid = *tid;
+	ItemPointer result;
+	AclResult	aclresult;
+	Snapshot	snapshot;
+	TableScanDesc scan;
+
+	result = (ItemPointer) palloc(sizeof(ItemPointerData));
+
+	aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
+  ACL_SELECT);
+	if (aclresult != ACLCHECK_OK)
+		aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
+	   RelationGetRelationName(rel));
+
+	if (rel->rd_rel->relkind == RELKIND_VIEW)
+		return currtid_for_view(rel, tid);
+
+	if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+		elog(ERROR, "cannot look at latest visible tid for relation \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(rel)),
+			 RelationGetRelationName(rel));
+
+	ItemPointerCopy(tid, result);
+
+	snapshot = RegisterSnapshot(GetLatestSnapshot());
+	scan = table_beginscan_tid(rel, snapshot);
+	table_tuple_get_latest_tid(scan, result);
+	table_endscan(scan);
+	UnregisterSnapshot(snapshot);
+
+	return result;
 }
 
 /*
@@ -288,7 +322,7 @@ setLastTid(const ItemPointer tid)
  *		CTID should be defined in the view and it must
  *		correspond to the CTID of a base relation.
  */
-static Datum
+static ItemPointer
 currtid_for_view(Relation viewrel, ItemPointer tid)
 {
 	TupleDesc	att = RelationGetDescr(viewrel);
@@ -338,12 +372,12 @@ currtid_for_view(Relation viewrel, ItemPointer tid)
 	rte = rt_fetch(var->varno, query->rtable);
 	if (rte)
 	{
-		Datum		result;
+		ItemPointer	result;
+		Relation	rel;
 
-		result = DirectFunctionCall2(currtid_byreloid,
-	 ObjectIdGetDatum(rte->relid),
-	 PointerGetDatum(tid));
-		table_close(viewrel, 

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> So, what you are basically saying is to switch currtid_byreloid() to
> become a function local to tid.c.  And then have just
> currtid_byrelname() and currtid_for_view() call that, right?

Yeah, that sounds about right.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Sat, Nov 21, 2020 at 01:13:35PM -0500, Tom Lane wrote:
> Considering that we're preserving this only for backwards compatibility,
> I doubt that changing the signature is a good idea.  It maybe risks
> breaking something, and the ODBC driver is hardly going to notice
> any improved ease-of-use.

So, what you are basically saying is to switch currtid_byreloid() to
become a function local to tid.c.  And then have just
currtid_byrelname() and currtid_for_view() call that, right?
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Andres Freund]

Hi,

+1 for getting rid of whatever we can without too much trouble.

On 2020-11-21 13:13:35 -0500, Tom Lane wrote:
> Michael Paquier  writes:
> > Indeed, this could go.  There is a recursive call for views, but in
> > order to maintain compatibility with that we can just remove one
> > function and move the second to use a regclass as argument, like the
> > attached, while removing setLastTid().  Any thoughts?
> 
> Considering that we're preserving this only for backwards compatibility,
> I doubt that changing the signature is a good idea.  It maybe risks
> breaking something, and the ODBC driver is hardly going to notice
> any improved ease-of-use.

+1.

Regards,

Andres

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> Indeed, this could go.  There is a recursive call for views, but in
> order to maintain compatibility with that we can just remove one
> function and move the second to use a regclass as argument, like the
> attached, while removing setLastTid().  Any thoughts?

Considering that we're preserving this only for backwards compatibility,
I doubt that changing the signature is a good idea.  It maybe risks
breaking something, and the ODBC driver is hardly going to notice
any improved ease-of-use.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Fri, Nov 20, 2020 at 09:50:08PM -0500, Tom Lane wrote:
> Michael Paquier  writes:
>> What about cutting the cake in two and just remove
>> currtid() then?
> 
> +1.  That'd still let us get rid of setLastTid() which is
> the ugliest part of the thing, IMO.

Indeed, this could go.  There is a recursive call for views, but in
order to maintain compatibility with that we can just remove one
function and move the second to use a regclass as argument, like the
attached, while removing setLastTid().  Any thoughts?
--
Michael
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 92b19dba32..54b2eb7378 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -129,7 +129,6 @@ extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
    bool *all_dead, bool first_call);
 
 extern void heap_get_latest_tid(TableScanDesc scan, ItemPointer tid);
-extern void setLastTid(const ItemPointer tid);
 
 extern BulkInsertState GetBulkInsertState(void);
 extern void FreeBulkInsertState(BulkInsertState);
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index c6da0df868..7d1f6ec234 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*			mmddN */
-#define CATALOG_VERSION_NO	202011191
+#define CATALOG_VERSION_NO	202011211
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 33dacfd340..63ddfe99d3 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2549,13 +2549,10 @@
 { oid => '1292',
   proname => 'tideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'tid tid', prosrc => 'tideq' },
-{ oid => '1293', descr => 'latest tid of a tuple',
-  proname => 'currtid', provolatile => 'v', proparallel => 'u',
-  prorettype => 'tid', proargtypes => 'oid tid', prosrc => 'currtid_byreloid' },
 { oid => '1294', descr => 'latest tid of a tuple',
   proname => 'currtid2', provolatile => 'v', proparallel => 'u',
-  prorettype => 'tid', proargtypes => 'text tid',
-  prosrc => 'currtid_byrelname' },
+  prorettype => 'tid', proargtypes => 'regclass tid',
+  prosrc => 'currtid_byreloid' },
 { oid => '1265',
   proname => 'tidne', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'tid tid', prosrc => 'tidne' },
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 29e07b7228..e0f24283b8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -645,10 +645,7 @@ ExecInsert(ModifyTableState *mtstate,
 	}
 
 	if (canSetTag)
-	{
 		(estate->es_processed)++;
-		setLastTid(>tts_tid);
-	}
 
 	/*
 	 * If this insert is the result of a partition key update that moved the
diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 509a0fdffc..4d191d91b6 100644
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -275,14 +275,6 @@ hashtidextended(PG_FUNCTION_ARGS)
  *	Maybe these implementations should be moved to another place
  */
 
-static ItemPointerData Current_last_tid = {{0, 0}, 0};
-
-void
-setLastTid(const ItemPointer tid)
-{
-	Current_last_tid = *tid;
-}
-
 /*
  *	Handle CTIDs of views.
  *		CTID should be defined in the view and it must
@@ -367,11 +359,6 @@ currtid_byreloid(PG_FUNCTION_ARGS)
 	TableScanDesc scan;
 
 	result = (ItemPointer) palloc(sizeof(ItemPointerData));
-	if (!reloid)
-	{
-		*result = Current_last_tid;
-		PG_RETURN_ITEMPOINTER(result);
-	}
 
 	rel = table_open(reloid, AccessShareLock);
 
@@ -401,46 +388,3 @@ currtid_byreloid(PG_FUNCTION_ARGS)
 
 	PG_RETURN_ITEMPOINTER(result);
 }
-
-Datum
-currtid_byrelname(PG_FUNCTION_ARGS)
-{
-	text	   *relname = PG_GETARG_TEXT_PP(0);
-	ItemPointer tid = PG_GETARG_ITEMPOINTER(1);
-	ItemPointer result;
-	RangeVar   *relrv;
-	Relation	rel;
-	AclResult	aclresult;
-	Snapshot	snapshot;
-	TableScanDesc scan;
-
-	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
-	rel = table_openrv(relrv, AccessShareLock);
-
-	aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
-  ACL_SELECT);
-	if (aclresult != ACLCHECK_OK)
-		aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
-	   RelationGetRelationName(rel));
-
-	if (rel->rd_rel->relkind == RELKIND_VIEW)
-		return currtid_for_view(rel, tid);
-
-	if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
-		elog(ERROR, "cannot look at latest visible tid for relation \"%s.%s\"",
-			 get_namespace_name(RelationGetNamespace(rel)),
-			 RelationGetRelationName(rel));
-
-	result = (ItemPointer) palloc(sizeof(ItemPointerData));
-	ItemPointerCopy(tid, result);
-
-	snapshot = RegisterSnapshot(GetLatestSnapshot());
-	scan = table_beginscan_tid(rel, snapshot);
-	table_tuple_get_latest_tid(scan, result);
-	table_endscan(scan);
-	UnregisterSnapshot(snapshot);
-
-	table_close(rel, AccessShareLock);
-
-	PG_RETURN_ITEMPOINTER(result);
-}
diff --git 

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> What about cutting the cake in two and just remove
> currtid() then?

+1.  That'd still let us get rid of setLastTid() which is
the ugliest part of the thing, IMO.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Fri, Nov 20, 2020 at 11:53:11AM -0500, Tom Lane wrote:
> Yeah, if pgODBC were not using it at all then I think it'd be fine
> to get rid of, but if it still contains calls then we cannot.
> The suggestion upthread that those calls might be unreachable
> is interesting, but it seems unproven.

Yeah, I am not 100% sure that there are no code paths calling
currtid2(), and the ODBC is too obscure to me to get to a clear
conclusion.  currtid() though, is a different deal thanks to
RETURNING.  What about cutting the cake in two and just remove
currtid() then?
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-09-03 12:14, Michael Paquier wrote:
>> Opinions are welcome about the arguments of upthread.

> It appears that currtid2() is still used, so we ought to keep it.

Yeah, if pgODBC were not using it at all then I think it'd be fine
to get rid of, but if it still contains calls then we cannot.
The suggestion upthread that those calls might be unreachable
is interesting, but it seems unproven.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Peter Eisentraut]

On 2020-09-03 12:14, Michael Paquier wrote:

On Fri, Jun 26, 2020 at 01:11:55PM +0900, Michael Paquier wrote:

 From what I can see on this thread, we could just remove currtid() per
the arguments of the RETURNING ctid clause supported since PG 8.2, but
it would make more sense to me to just remove both currtid/currtid2()
at once.


The CF bot is complaining, so here is a rebase for the main patch.
Opinions are welcome about the arguments of upthread.


It appears that currtid2() is still used, so we ought to keep it.

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Fri, Jun 26, 2020 at 01:11:55PM +0900, Michael Paquier wrote:
> From what I can see on this thread, we could just remove currtid() per
> the arguments of the RETURNING ctid clause supported since PG 8.2, but
> it would make more sense to me to just remove both currtid/currtid2()
> at once.

The CF bot is complaining, so here is a rebase for the main patch.
Opinions are welcome about the arguments of upthread.
--
Michael
From 9da62abad29e5b2b8a8b24498eba7a2c3f718f9b Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 3 Jun 2020 10:49:41 +0900
Subject: [PATCH v2] Remove currtid() and currtid2()

Those two functions have been used within the Postgres ODBC driver in
the old days, requiring them when connecting to Postgres 8.1 or older.
In 8.2, support for RETURNING has allowed the driver to fetch ctids
by directly using this clause.
---
 src/include/access/heapam.h|   1 -
 src/include/catalog/pg_proc.dat|   7 -
 src/backend/executor/nodeModifyTable.c |   1 -
 src/backend/utils/adt/tid.c| 177 -
 src/test/regress/expected/tid.out  |  88 
 src/test/regress/sql/tid.sql   |  52 
 6 files changed, 326 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 92b19dba32..54b2eb7378 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -129,7 +129,6 @@ extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
    bool *all_dead, bool first_call);
 
 extern void heap_get_latest_tid(TableScanDesc scan, ItemPointer tid);
-extern void setLastTid(const ItemPointer tid);
 
 extern BulkInsertState GetBulkInsertState(void);
 extern void FreeBulkInsertState(BulkInsertState);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 687509ba92..d9ea8c8244 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2542,13 +2542,6 @@
 { oid => '1292',
   proname => 'tideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'tid tid', prosrc => 'tideq' },
-{ oid => '1293', descr => 'latest tid of a tuple',
-  proname => 'currtid', provolatile => 'v', proparallel => 'u',
-  prorettype => 'tid', proargtypes => 'oid tid', prosrc => 'currtid_byreloid' },
-{ oid => '1294', descr => 'latest tid of a tuple',
-  proname => 'currtid2', provolatile => 'v', proparallel => 'u',
-  prorettype => 'tid', proargtypes => 'text tid',
-  prosrc => 'currtid_byrelname' },
 { oid => '1265',
   proname => 'tidne', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'tid tid', prosrc => 'tidne' },
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 20a4c474cc..fc53a12d56 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -629,7 +629,6 @@ ExecInsert(ModifyTableState *mtstate,
 	if (canSetTag)
 	{
 		(estate->es_processed)++;
-		setLastTid(>tts_tid);
 	}
 
 	/*
diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 509a0fdffc..29dddb20e6 100644
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -267,180 +267,3 @@ hashtidextended(PG_FUNCTION_ARGS)
 			 sizeof(BlockIdData) + sizeof(OffsetNumber),
 			 seed);
 }
-
-
-/*
- *	Functions to get latest tid of a specified tuple.
- *
- *	Maybe these implementations should be moved to another place
- */
-
-static ItemPointerData Current_last_tid = {{0, 0}, 0};
-
-void
-setLastTid(const ItemPointer tid)
-{
-	Current_last_tid = *tid;
-}
-
-/*
- *	Handle CTIDs of views.
- *		CTID should be defined in the view and it must
- *		correspond to the CTID of a base relation.
- */
-static Datum
-currtid_for_view(Relation viewrel, ItemPointer tid)
-{
-	TupleDesc	att = RelationGetDescr(viewrel);
-	RuleLock   *rulelock;
-	RewriteRule *rewrite;
-	int			i,
-natts = att->natts,
-tididx = -1;
-
-	for (i = 0; i < natts; i++)
-	{
-		Form_pg_attribute attr = TupleDescAttr(att, i);
-
-		if (strcmp(NameStr(attr->attname), "ctid") == 0)
-		{
-			if (attr->atttypid != TIDOID)
-elog(ERROR, "ctid isn't of type TID");
-			tididx = i;
-			break;
-		}
-	}
-	if (tididx < 0)
-		elog(ERROR, "currtid cannot handle views with no CTID");
-	rulelock = viewrel->rd_rules;
-	if (!rulelock)
-		elog(ERROR, "the view has no rules");
-	for (i = 0; i < rulelock->numLocks; i++)
-	{
-		rewrite = rulelock->rules[i];
-		if (rewrite->event == CMD_SELECT)
-		{
-			Query	   *query;
-			TargetEntry *tle;
-
-			if (list_length(rewrite->actions) != 1)
-elog(ERROR, "only one select rule is allowed in views");
-			query = (Query *) linitial(rewrite->actions);
-			tle = get_tle_by_resno(query->targetList, tididx + 1);
-			if (tle && tle->expr && IsA(tle->expr, Var))
-			{
-Var		   *var = (Var *) tle->expr;
-RangeTblEntry *rte;
-
-if (!IS_SPECIAL_VARNO(var->varno) &&
-	var->varattno == SelfItemPointerAttributeNumber)
-{
-	rte = 

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Thu, Jun 25, 2020 at 10:14:00PM +0900, Inoue, Hiroshi wrote:
> I seem to have invalidated KEYSET-DRIVEN cursors used in positioned-update
> test.  It was introduced by the commit 4a272fd but was invalidated by the
> commit 2be35a6.
> 
> I don't object to the removal of currtid(2) because keyset-driven cursors in
> psqlodbc are changed into static cursors in many cases and I've hardly ever
> heard a complaint about it.

Hmm.  I am not sure that this completely answers my original concern
though.  In short, don't we still have corner cases where
keyset-driven cursors are not changed into static cursors, meaning
that currtid2() could get used?  The removal of the in-core functions
would hurt applications using that, meaning that we should at least
provide an equivalent of currtid2() in the worse case as a contrib
module, no?  If the code paths of currtid2() are reachable, shouldn't
we also make sure that they are still reached in the regression tests
of the driver, meaning that the driver code needs more coverage?  I
have been looking at the tests and tried to tweak them using
SQLSetPos() so as the code paths involving currtid2() get reached, but 
I am not really able to do so.  It does not mean that that currtid2()
never gets reached, it just means that I am not able to be sure that
this part can be safely removed from the Postgres backend code :( 

From what I can see on this thread, we could just remove currtid() per
the arguments of the RETURNING ctid clause supported since PG 8.2, but
it would make more sense to me to just remove both currtid/currtid2()
at once.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Inoue, Hiroshi]

Hi,

I seem to have invalidated KEYSET-DRIVEN cursors used in 
positioned-update test .
It was introduced by the commit 4a272fd but was invalidated by the 
commit 2be35a6.


I don't object to the removal of currtid(2) because keyset-driven 
cursors in psqlodbc are changed into static cursors in many cases and 
I've hardly ever heard a complaint about it.


regards,
Hiroshi Inoue

On 2020/06/24 11:11, Michael Paquier wrote:

On Tue, Jun 23, 2020 at 02:02:33PM +0900, Michael Paquier wrote:

Actually, while reviewing the code, the only code path where we use
currtid2() involves positioned_load() and LATEST_TUPLE_LOAD.  And the
only location where this happens is in SC_pos_reload_with_key(), where
I don't actually see how it would be possible to not have a keyset and
still use a CTID, which would led to LATEST_TUPLE_LOAD being used.  So
could it be possible that the code paths of currtid2() are actually
just dead code?

I have dug more into this one, and we actually stressed this code path
quite a lot up to commit d9cb23f in the ODBC driver, with tests
cursor-block-delete, positioned-update and bulkoperations particularly
when calling SQLSetPos().  However, 86e2e7a has reworked the code in
such a way that we visibly don't use anymore CTIDs if we don't have a
keyset, and that combinations of various options like UseDeclareFetch
or UpdatableCursors don't trigger this code path anymore.  In short,
currtid2() does not get used.  Inoue-san, Saito-san, what do you
think?  I am adding also Tsunakawa-san in CC who has some experience
in this area.
--
Michael

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

Hi Inoue-san,

On Wed, Jun 24, 2020 at 05:20:42PM +0900, Inoue, Hiroshi wrote:
> Where do you test, on Windows or on *nix?
> How do you test there?

I have been testing the driver on macos only, with various backend
versions, from 11 to 14.

Thanks,
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Inoue, Hiroshi]

Hi Michael,

Where do you test, on Windows or on *nix?
How do you test there?

regards,
Hiroshi Inoue

On 2020/06/24 11:11, Michael Paquier wrote:

On Tue, Jun 23, 2020 at 02:02:33PM +0900, Michael Paquier wrote:

Actually, while reviewing the code, the only code path where we use
currtid2() involves positioned_load() and LATEST_TUPLE_LOAD.  And the
only location where this happens is in SC_pos_reload_with_key(), where
I don't actually see how it would be possible to not have a keyset and
still use a CTID, which would led to LATEST_TUPLE_LOAD being used.  So
could it be possible that the code paths of currtid2() are actually
just dead code?

I have dug more into this one, and we actually stressed this code path
quite a lot up to commit d9cb23f in the ODBC driver, with tests
cursor-block-delete, positioned-update and bulkoperations particularly
when calling SQLSetPos().  However, 86e2e7a has reworked the code in
such a way that we visibly don't use anymore CTIDs if we don't have a
keyset, and that combinations of various options like UseDeclareFetch
or UpdatableCursors don't trigger this code path anymore.  In short,
currtid2() does not get used.  Inoue-san, Saito-san, what do you
think?  I am adding also Tsunakawa-san in CC who has some experience
in this area.
--
Michael

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Tue, Jun 23, 2020 at 02:02:33PM +0900, Michael Paquier wrote:
> Actually, while reviewing the code, the only code path where we use
> currtid2() involves positioned_load() and LATEST_TUPLE_LOAD.  And the
> only location where this happens is in SC_pos_reload_with_key(), where
> I don't actually see how it would be possible to not have a keyset and
> still use a CTID, which would led to LATEST_TUPLE_LOAD being used.  So
> could it be possible that the code paths of currtid2() are actually
> just dead code?

I have dug more into this one, and we actually stressed this code path
quite a lot up to commit d9cb23f in the ODBC driver, with tests
cursor-block-delete, positioned-update and bulkoperations particularly
when calling SQLSetPos().  However, 86e2e7a has reworked the code in
such a way that we visibly don't use anymore CTIDs if we don't have a
keyset, and that combinations of various options like UseDeclareFetch
or UpdatableCursors don't trigger this code path anymore.  In short,
currtid2() does not get used.  Inoue-san, Saito-san, what do you
think?  I am adding also Tsunakawa-san in CC who has some experience
in this area.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Tue, Jun 23, 2020 at 01:29:06PM +0900, Michael Paquier wrote:
> Sorry, but I am not quite sure what is the relationship between
> UseDeclareFetch and currtid2()?  Is that related to the use of
> SQL_CURSOR_KEYSET_DRIVEN?  The only thing I can be sure of here is
> that we never call currtid2() in any of the regression tests present
> in the ODBC code for any of the scenarios covered by installcheck-all,
> so that does not really bring any confidence that removing currtid2()
> is a wise thing to do, because we may silently break stuff.  If the
> function is used, it would be good to close the gap with a test to
> stress that at least in the driver.

Actually, while reviewing the code, the only code path where we use
currtid2() involves positioned_load() and LATEST_TUPLE_LOAD.  And the
only location where this happens is in SC_pos_reload_with_key(), where
I don't actually see how it would be possible to not have a keyset and
still use a CTID, which would led to LATEST_TUPLE_LOAD being used.  So
could it be possible that the code paths of currtid2() are actually
just dead code?
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Mon, Jun 15, 2020 at 08:50:23PM +0900, Inoue, Hiroshi wrote:
> Sorry for the reply.

No problem, thanks for taking the time.

> On 2020/06/08 15:52, Michael Paquier wrote:
>> On Fri, Jun 05, 2020 at 10:25:00PM +0900, Inoue, Hiroshi wrote:
>>We have two
>> problems here then:
>> 1) We cannot actually really remove currtid2() from the backend yet
>> without removing the dependency in the driver, or that may break some
>> users.
> 
> I think only ODBC driver uses currtid2().

Check.  I think so too.

>> 2) The driver does not include tests for that stuff yet.
> 
> SQLSetPos(.., .., SQL_REFRESH, ..) call in positioned-update-test passes the
> stuff
>  when 'Use Declare/Fetch' option is turned off. In other words,
> keyset-driven cursor
> is not supported when 'Use Declare/Fetch' option is turned on. Probably
> keyset-driven
> cursor support would be lost regardless of 'Use Declare/Fetch' option after
> the removal of currtid2().

Sorry, but I am not quite sure what is the relationship between
UseDeclareFetch and currtid2()?  Is that related to the use of
SQL_CURSOR_KEYSET_DRIVEN?  The only thing I can be sure of here is
that we never call currtid2() in any of the regression tests present
in the ODBC code for any of the scenarios covered by installcheck-all,
so that does not really bring any confidence that removing currtid2()
is a wise thing to do, because we may silently break stuff.  If the
function is used, it would be good to close the gap with a test to
stress that at least in the driver.

currtid(), on the contrary, would be fine as far as I understand
because the ODBC code relies on a RETURNING ctid instead, and that's
supported for ages in the Postgres backend.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Inoue, Hiroshi]

Sorry for the reply.

On 2020/06/08 15:52, Michael Paquier wrote:

On Fri, Jun 05, 2020 at 10:25:00PM +0900, Inoue, Hiroshi wrote:

Keyset-driven cursors always detect changes made by other applications
(and themselves). currtid() is necessary to detect the changes.
CTIDs are changed by updates unfortunately.

You mean currtid2() here and not currtid(), right?


Yes.


   We have two
problems here then:
1) We cannot actually really remove currtid2() from the backend yet
without removing the dependency in the driver, or that may break some
users.


I think only ODBC driver uses currtid2().


2) The driver does not include tests for that stuff yet.


SQLSetPos(.., .., SQL_REFRESH, ..) call in positioned-update-test passes 
the stuff
 when 'Use Declare/Fetch' option is turned off. In other words, 
keyset-driven cursor
is not supported when 'Use Declare/Fetch' option is turned on. Probably 
keyset-driven
cursor support would be lost regardless of 'Use Declare/Fetch' option 
after the

removal of currtid2().


--
Michael

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Fri, Jun 05, 2020 at 10:25:00PM +0900, Inoue, Hiroshi wrote:
> Keyset-driven cursors always detect changes made by other applications
> (and themselves). currtid() is necessary to detect the changes.
> CTIDs are changed by updates unfortunately.

You mean currtid2() here and not currtid(), right?  We have two
problems here then:
1) We cannot actually really remove currtid2() from the backend yet
without removing the dependency in the driver, or that may break some
users.
2) The driver does not include tests for that stuff yet.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Inoue, Hiroshi]

On 2020/06/05 15:22, Michael Paquier wrote:

On Wed, Jun 03, 2020 at 10:10:21PM +0900, Inoue, Hiroshi wrote:

On 2020/06/03 11:14, Michael Paquier wrote:

I have been looking at the ODBC driver and the need for currtid() as
well as currtid2(), and as mentioned already in [1], matching with my
lookup of things, these are actually not needed by the driver as long
as we connect to a server newer than 8.2 able to support RETURNING.

Though currtid2() is necessary even for servers which support RETURNING,
I don't object to remove it.

In which cases is it getting used then?


Keyset-driven cursors always detect changes made by other applications
(and themselves). currtid() is necessary to detect the changes.
CTIDs are changed by updates unfortunately.

regards,
Hiroshi Inoue


   From what I can see there is
zero coverage for that part in the tests.  And based on a rough read
of the code, this would get called with LATEST_TUPLE_LOAD being set,
where there is some kind of bulk deletion involved.  Couldn't that be
a problem?
--
Michael

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Wed, Jun 03, 2020 at 10:10:21PM +0900, Inoue, Hiroshi wrote:
> On 2020/06/03 11:14, Michael Paquier wrote:
>> I have been looking at the ODBC driver and the need for currtid() as
>> well as currtid2(), and as mentioned already in [1], matching with my
>> lookup of things, these are actually not needed by the driver as long
>> as we connect to a server newer than 8.2 able to support RETURNING.
> 
> Though currtid2() is necessary even for servers which support RETURNING,
> I don't object to remove it.

In which cases is it getting used then?  From what I can see there is
zero coverage for that part in the tests.  And based on a rough read
of the code, this would get called with LATEST_TUPLE_LOAD being set,
where there is some kind of bulk deletion involved.  Couldn't that be
a problem?
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Thu, Jun 04, 2020 at 10:59:05AM -0700, Andres Freund wrote:
> What's the point of that change? I think the differentiation between
> heapam_handler.c and heapam.c could be clearer, but if anything, I'd
> argue that heap_get_latest_tid is sufficiently low-level that it'd
> belong in heapam.c.

Well, heap_get_latest_tid() is only called in heapam_handler.c if
anything, as it is not used elsewhere and not publish it.  And IMO we
should try to encourage using table_get_latest_tid() instead if some
plugins need that.  Anyway, if you are opposed to this change, I won't
push hard for it either.
--
Michael


signature.asc
Description: PGP signature

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Andres Freund]

Hi,

On 2020-06-03 11:14:48 +0900, Michael Paquier wrote:
> I would like to remove those two functions and the surrounding code
> for v14, leading to some cleanup:
>  6 files changed, 326 deletions(-)

+1


> While on it, I have noticed that heap_get_latest_tid() is still
> located within heapam.c, but we can just move it within
> heapam_handler.c.

What's the point of that change? I think the differentiation between
heapam_handler.c and heapam.c could be clearer, but if anything, I'd
argue that heap_get_latest_tid is sufficiently low-level that it'd
belong in heapam.c.


Greetings,

Andres Freund

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Inoue, Hiroshi]

Hi,

On 2020/06/03 11:14, Michael Paquier wrote:

Hi all,



I have been looking at the ODBC driver and the need for currtid() as
well as currtid2(), and as mentioned already in [1], matching with my
lookup of things, these are actually not needed by the driver as long
as we connect to a server newer than 8.2 able to support RETURNING.


Though currtid2() is necessary even for servers which support RETURNING,
I don't object to remove it.

regards,
Hiroshi Inoue


   I
am adding in CC of this thread Saito-san and Inoue-san who are the
two main maintainers of the driver for comments.  It is worth noting
that on its latest HEAD the ODBC driver requires libpq from at least
9.2.

I would like to remove those two functions and the surrounding code
for v14, leading to some cleanup:
  6 files changed, 326 deletions(-)

While on it, I have noticed that heap_get_latest_tid() is still
located within heapam.c, but we can just move it within
heapam_handler.c.

Attached are two patches to address both points.  Comments are
welcome.

Thanks,

[1]: 
https://www.postgresql.org/message-id/20200529005559.jl2gsolomyro4...@alap3.anarazel.de
--
Michael

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

I wrote:
> It looks like table_beginscan_tid wouldn't need to be exported anymore
> either.

Ah, scratch that, I misread it.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> I would like to remove those two functions and the surrounding code
> for v14, leading to some cleanup:

+1

> While on it, I have noticed that heap_get_latest_tid() is still
> located within heapam.c, but we can just move it within
> heapam_handler.c.

It looks like table_beginscan_tid wouldn't need to be exported anymore
either.

regards, tom lane