Small and unaffected typo in pg_logical_slot_get_changes_guts()

Started by Kasahara Tatsuhitoalmost 4 years ago9 messages

kasahara.tatsuhito@gmail.com

almost 4 years ago

Hi,

I noticed the small typo in pg_logical_slot_get_changes_guts().

======================================
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -295,7 +295,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo
fcinfo, bool confirm, bool bin
                        CHECK_FOR_INTERRUPTS();
                }

-               tuplestore_donestoring(tupstore);
+               tuplestore_donestoring(p->tupstore);
======================================

As above, I think tuplestore_donestoring() should have p->tupstore as
an argument.
Fortunately, tuplestore_donestoring() is defined as ((void) 0) and
does not do anything. Therefore, this typo has no effect

However, it is better to fix the typo or remove the
tuplestore_donestoring() part to avoid confusion.

Since commit dd04e958c8b03c0f0512497651678c7816af3198,
tuplestore_donestoring() seems to be left only for compatibility, so
it looks like it can be removed from the core code, except for the
header (tuplestore.h).

Best regards,

--
Tatsuhito Kasahara
kasahara.tatsuhito _at_ gmail.com

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Kasahara Tatsuhito (#1)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

On Wed, Feb 16, 2022 at 11:27:58AM +0900, Kasahara Tatsuhito wrote:

Since commit dd04e958c8b03c0f0512497651678c7816af3198,
tuplestore_donestoring() seems to be left only for compatibility, so
it looks like it can be removed from the core code, except for the
header (tuplestore.h).

FWIW, it looks sensible to me to just do the cleanup you are
suggesting here on HEAD, aka keeping the compatibility macro in the
header, but wiping out any references of it in the C code. That
reduces the chances of copy-pasting it around for no effect. Would
you like to write a patch?
--
Michael

Kasahara Tatsuhito

kasahara.tatsuhito@gmail.com

almost 4 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

Hi !

2022年2月16日(水) 11:42 Michael Paquier <michael@paquier.xyz>:

On Wed, Feb 16, 2022 at 11:27:58AM +0900, Kasahara Tatsuhito wrote:

Since commit dd04e958c8b03c0f0512497651678c7816af3198,
tuplestore_donestoring() seems to be left only for compatibility, so
it looks like it can be removed from the core code, except for the
header (tuplestore.h).

FWIW, it looks sensible to me to just do the cleanup you are
suggesting here on HEAD, aka keeping the compatibility macro in the
header, but wiping out any references of it in the C code. That
reduces the chances of copy-pasting it around for no effect. Would
you like to write a patch?

Thanks for the reply. The patch is attached.
Remove all references to tuplestore_donestoring() except for the header.

Best regards,

--
Tatsuhito Kasahara
kasahara.tatsuhito _at_ gmail.com

Attachments:

remove_tuplestore_donestoring.patchapplication/octet-stream; name=remove_tuplestore_donestoring.patchDownload

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 5a37508..efc4c94 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1005,8 +1005,6 @@ materializeResult(FunctionCallInfo fcinfo, PGconn *conn, PGresult *res)
 			/* clean up GUC settings, if we changed any */
 			restoreLocalGucs(nestlevel);
 
-			/* clean up and return the tuplestore */
-			tuplestore_donestoring(tupstore);
 		}
 	}
 	PG_FINALLY();
@@ -1988,9 +1986,6 @@ dblink_get_notify(PG_FUNCTION_ARGS)
 		PQconsumeInput(conn);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index f1e64a39..50892b5 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -325,9 +325,7 @@ brin_page_items(PG_FUNCTION_ARGS)
 			break;
 	}
 
-	/* clean up and return the tuplestore */
 	brin_free_desc(bdesc);
-	tuplestore_donestoring(tupstore);
 	index_close(indexRel, AccessShareLock);
 
 	return (Datum) 0;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 082bfa8..9d7d081 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1803,13 +1803,11 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
 	LWLockRelease(pgss->lock);
 
 	if (qbuffer)
 		free(qbuffer);
 
-	tuplestore_donestoring(tupstore);
 }
 
 /* Number of output arguments (columns) for pg_stat_statements_info */
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 29fcb6a..f753c6e 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1508,12 +1508,7 @@ postgres_fdw_get_connections(PG_FUNCTION_ARGS)
 
 	/* If cache doesn't exist, we return no records */
 	if (!ConnectionHash)
-	{
-		/* clean up and return the tuplestore */
-		tuplestore_donestoring(tupstore);
-
 		PG_RETURN_VOID();
-	}
 
 	hash_seq_init(&scan, ConnectionHash);
 	while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
@@ -1578,8 +1573,6 @@ postgres_fdw_get_connections(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
 
 	PG_RETURN_VOID();
 }
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index afbbdfc..e308228 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -943,8 +943,6 @@ get_crosstab_tuplestore(char *sql,
 		/* internal error */
 		elog(ERROR, "get_crosstab_tuplestore: SPI_finish() failed");
 
-	tuplestore_donestoring(tupstore);
-
 	return tupstore;
 }
 
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 7fdde8e..a2e5fb5 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -783,8 +783,6 @@ xpath_table(PG_FUNCTION_ARGS)
 
 	pg_xml_done(xmlerrcxt, false);
 
-	tuplestore_donestoring(tupstore);
-
 	SPI_finish();
 
 	rsinfo->setResult = tupstore;
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index d8af5aa..4a91153 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -252,7 +252,6 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 	values[0] = LSNGetDatum(stoppoint);
 
 	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-	tuplestore_donestoring(tupstore);
 
 	return (Datum) 0;
 }
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 93c2099..1e85875 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1401,9 +1401,6 @@ pg_event_trigger_dropped_objects(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
@@ -2061,9 +2058,6 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	PG_RETURN_VOID();
 }
 
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index a2e77c4..0e04304 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -2021,9 +2021,6 @@ pg_available_extensions(PG_FUNCTION_ARGS)
 		FreeDir(dir);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
@@ -2112,9 +2109,6 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
 		FreeDir(dir);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
@@ -2417,9 +2411,6 @@ pg_extension_update_paths(PG_FUNCTION_ARGS)
 		}
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 206d2bb..e0c985e 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -778,9 +778,6 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
 		}
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	rsinfo->returnMode = SFRM_Materialize;
 	rsinfo->setResult = tupstore;
 	rsinfo->setDesc = tupdesc;
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 294e22c..d910bc2 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -555,9 +555,6 @@ deflist_to_tuplestore(ReturnSetInfo *rsinfo, List *options)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	MemoryContextSwitchTo(oldcontext);
 }
 
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 7b47390..5a68d6d 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -1022,8 +1022,5 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
 
 	LWLockRelease(LogicalRepWorkerLock);
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 4d71e71..d7a7313 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -295,8 +295,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 			CHECK_FOR_INTERRUPTS();
 		}
 
-		tuplestore_donestoring(tupstore);
-
 		/*
 		 * Logical decoding could have clobbered CurrentResourceOwner during
 		 * transaction management, so restore the executor's value.  (This is
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index e91fa93..76055a8 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1568,8 +1568,6 @@ pg_show_replication_origin_status(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	tuplestore_donestoring(tupstore);
-
 	LWLockRelease(ReplicationOriginLock);
 
 #undef REPLICATION_ORIGIN_PROGRESS_COLS
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index ae6316d..7952837 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -435,8 +435,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 
 	LWLockRelease(ReplicationSlotControlLock);
 
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 655760f..8a78c01 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -3579,9 +3579,6 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index c682775..1f023a3 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -605,7 +605,5 @@ pg_get_shmem_allocations(PG_FUNCTION_ARGS)
 
 	LWLockRelease(ShmemIndexLock);
 
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 28cb9d3..c7c95ad 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -152,9 +152,6 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
 	PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
 									 TopMemoryContext, NULL, 0);
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 15cb17a..30e8dfa 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -555,9 +555,6 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
@@ -953,9 +950,6 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			break;
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
@@ -1936,9 +1930,6 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index b73cebf..eda9c1e 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -4855,8 +4855,6 @@ text_to_table(PG_FUNCTION_ARGS)
 
 	(void) split_text(fcinfo, &tstate);
 
-	tuplestore_donestoring(tstate.tupstore);
-
 	rsi->returnMode = SFRM_Materialize;
 	rsi->setResult = tstate.tupstore;
 	rsi->setDesc = tstate.tupdesc;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e2fe219..5aa835d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10205,8 +10205,6 @@ show_all_file_settings(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	tuplestore_donestoring(tupstore);
-
 	return (Datum) 0;
 }
 
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index 7a13212..d916d7b 100644
--- a/src/backend/utils/misc/pg_config.c
+++ b/src/backend/utils/misc/pg_config.c
@@ -85,7 +85,6 @@ pg_config(PG_FUNCTION_ARGS)
 	 */
 	ReleaseTupleDesc(tupdesc);
 
-	tuplestore_donestoring(tupstore);
 	rsinfo->setResult = tupstore;
 
 	/*
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 236f450..7885344 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -1204,9 +1204,6 @@ pg_cursor(PG_FUNCTION_ARGS)
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 
-	/* clean up and return the tuplestore */
-	tuplestore_donestoring(tupstore);
-
 	rsinfo->returnMode = SFRM_Materialize;
 	rsinfo->setResult = tupstore;
 	rsinfo->setDesc = tupdesc;

Justin Pryzby

pryzby@telsasoft.com

almost 4 years ago

In reply to: Kasahara Tatsuhito (#1)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

On Wed, Feb 16, 2022 at 11:27:58AM +0900, Kasahara Tatsuhito wrote:

-               tuplestore_donestoring(tupstore);
+               tuplestore_donestoring(p->tupstore);

Melanie's tuplestore patch also removes the bogus line.
/messages/by-id/20220106005748.GT14051@telsasoft.com

--
Justin

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Justin Pryzby (#4)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

On Tue, Feb 15, 2022 at 11:23:29PM -0600, Justin Pryzby wrote:

On Wed, Feb 16, 2022 at 11:27:58AM +0900, Kasahara Tatsuhito wrote:
-               tuplestore_donestoring(tupstore);
+               tuplestore_donestoring(p->tupstore);
Melanie's tuplestore patch also removes the bogus line.
/messages/by-id/20220106005748.GT14051@telsasoft.com

Thanks for the pointer! Kasahara-san's patch is doing the job, and I
can see that this does not really influence Melanie's patch, so I
would suggest to move on with this clean up. I'll go ping the other
thread about that part, first, of course.
--
Michael

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Kasahara Tatsuhito (#3)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

On Wed, Feb 16, 2022 at 01:25:09PM +0900, Kasahara Tatsuhito wrote:

Remove all references to tuplestore_donestoring() except for the header.

Looks fine, thanks. This has no impact on Melanie's patch posted on
[1]: /messages/by-id/20220106005748.GT14051@telsasoft.com -- Michael

[1]: /messages/by-id/20220106005748.GT14051@telsasoft.com -- Michael
--
Michael

Kasahara Tatsuhito

kasahara.tatsuhito@gmail.com

almost 4 years ago

In reply to: Michael Paquier (#6)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

2022年2月17日(木) 10:56 Michael Paquier <michael@paquier.xyz>:

On Wed, Feb 16, 2022 at 01:25:09PM +0900, Kasahara Tatsuhito wrote:

Remove all references to tuplestore_donestoring() except for the header.

Looks fine, thanks. This has no impact on Melanie's patch posted on
[1], so applied after tweaking the comment in tuplestore.h.

[1]: /messages/by-id/20220106005748.GT14051@telsasoft.com

Thanks !

Best regards,
--
Tatsuhito Kasahara
kasahara.tatsuhito _at_ gmail.com

Dagfinn Ilmari Mannsåker

ilmari@ilmari.org

almost 4 years ago

In reply to: Michael Paquier (#6)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Feb 16, 2022 at 01:25:09PM +0900, Kasahara Tatsuhito wrote:

Remove all references to tuplestore_donestoring() except for the header.

Looks fine, thanks. This has no impact on Melanie's patch posted on
[1], so applied after tweaking the comment in tuplestore.h.

Would it be possible to make that macro only defined when building
extensions, but not when building Postgres itself? For example, Perl
has a PERL_CORE macro that's only defined when building Perl itself, but
not when building CPAN modules, and backwards-compatibility macros and
functions are guarded by `#ifndef PERL_CORE`.

- ilmari

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Dagfinn Ilmari Mannsåker (#8)

Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

On Thu, Feb 17, 2022 at 11:33:55AM +0000, Dagfinn Ilmari Mannsåker wrote:

Would it be possible to make that macro only defined when building
extensions, but not when building Postgres itself? For example, Perl
has a PERL_CORE macro that's only defined when building Perl itself, but
not when building CPAN modules, and backwards-compatibility macros and
functions are guarded by `#ifndef PERL_CORE`.

That could be interesting in the long term, as compatibility macros
have accumulated over the years and there are from time to time
patches that are merged that make use of things they should not.

With the build we have now, I am not completely sure how you would do
the split though. Extensions are one thing, and we could set a flag
as part of USE_PGXS. There is also the popular case where extensions
are copied into the tree where we would not have USE_PGXS for them.
That would cause compilation failures. Another limit that could be
defined is to limit the removal of the compatibility macros for
src/backend/ and src/bin/, leaving any in-core modules out of the
picture.
--
Michael