Do away with a few backwards compatibility macros
Hi,
After a recent commit 6a72c42f (a related discussion [1]/messages/by-id/20231114175953.GD2062604@nathanxps13) which
removed MemoryContextResetAndDeleteChildren(), I think there are a
couple of other backward compatibility macros out there that can be
removed. These macros are tuplestore_donestoring() which was
introduced by commit dd04e95 21 years ago and SPI_push() and friends
which were made no-ops macros by commit 1833f1a 7 years ago. Debian
code search shows very minimal usages of these macros. Here's a patch
attached to remove them.
Thoughts?
[1]: /messages/by-id/20231114175953.GD2062604@nathanxps13
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Do-away-with-a-few-backwards-compatibility-macros.patchapplication/octet-stream; name=v1-0001-Do-away-with-a-few-backwards-compatibility-macros.patchDownload
From 2049247dde2c675c704ed971fd1d67a2db937bf6 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 16 Nov 2023 13:40:56 +0000
Subject: [PATCH v1] Do away with a few backwards compatibility macros
---
src/include/executor/spi.h | 7 -------
src/include/utils/tuplestore.h | 3 ---
2 files changed, 10 deletions(-)
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index d1de139a3b..d5f04d7275 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -100,13 +100,6 @@ typedef struct _SPI_plan *SPIPlanPtr;
#define SPI_OPT_NONATOMIC (1 << 0)
-/* These used to be functions, now just no-ops for backwards compatibility */
-#define SPI_push() ((void) 0)
-#define SPI_pop() ((void) 0)
-#define SPI_push_conditional() false
-#define SPI_pop_conditional(pushed) ((void) 0)
-#define SPI_restore_connection() ((void) 0)
-
extern PGDLLIMPORT uint64 SPI_processed;
extern PGDLLIMPORT SPITupleTable *SPI_tuptable;
extern PGDLLIMPORT int SPI_result;
diff --git a/src/include/utils/tuplestore.h b/src/include/utils/tuplestore.h
index 1077c5fdea..d4fbcbd96c 100644
--- a/src/include/utils/tuplestore.h
+++ b/src/include/utils/tuplestore.h
@@ -56,9 +56,6 @@ extern void tuplestore_puttuple(Tuplestorestate *state, HeapTuple tuple);
extern void tuplestore_putvalues(Tuplestorestate *state, TupleDesc tdesc,
const Datum *values, const bool *isnull);
-/* Backwards compatibility macro */
-#define tuplestore_donestoring(state) ((void) 0)
-
extern int tuplestore_alloc_read_pointer(Tuplestorestate *state, int eflags);
extern void tuplestore_select_read_pointer(Tuplestorestate *state, int ptr);
--
2.34.1
On Thu, Nov 16, 2023 at 07:11:41PM +0530, Bharath Rupireddy wrote:
After a recent commit 6a72c42f (a related discussion [1]) which
removed MemoryContextResetAndDeleteChildren(), I think there are a
couple of other backward compatibility macros out there that can be
removed. These macros are tuplestore_donestoring() which was
introduced by commit dd04e95 21 years ago and SPI_push() and friends
which were made no-ops macros by commit 1833f1a 7 years ago. Debian
code search shows very minimal usages of these macros. Here's a patch
attached to remove them.
I'm fine with this because all of these macros are no-ops for all supported
versions of Postgres. Even if an extension is using them today, you'll get
the same behavior as before if you remove the uses and rebuild against
v12-v16.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, Nov 16, 2023 at 09:46:22AM -0600, Nathan Bossart wrote:
On Thu, Nov 16, 2023 at 07:11:41PM +0530, Bharath Rupireddy wrote:
After a recent commit 6a72c42f (a related discussion [1]) which
removed MemoryContextResetAndDeleteChildren(), I think there are a
couple of other backward compatibility macros out there that can be
removed. These macros are tuplestore_donestoring() which was
introduced by commit dd04e95 21 years ago and SPI_push() and friends
which were made no-ops macros by commit 1833f1a 7 years ago. Debian
code search shows very minimal usages of these macros. Here's a patch
attached to remove them.I'm fine with this because all of these macros are no-ops for all supported
versions of Postgres. Even if an extension is using them today, you'll get
the same behavior as before if you remove the uses and rebuild against
v12-v16.
Barring objections, I'll plan on committing this in the next week or so.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
On Thu, Nov 16, 2023 at 09:46:22AM -0600, Nathan Bossart wrote:
I'm fine with this because all of these macros are no-ops for all supported
versions of Postgres. Even if an extension is using them today, you'll get
the same behavior as before if you remove the uses and rebuild against
v12-v16.
Barring objections, I'll plan on committing this in the next week or so.
No objection here, but should we try to establish some sort of project
policy around this sort of change (ie, removal of backwards-compatibility
support)? "Once it no longer matters for any supported version" sounds
about right to me, but maybe somebody has an argument for thinking about
it differently.
regards, tom lane
On Tue, Nov 21, 2023 at 12:05:36AM -0500, Tom Lane wrote:
No objection here, but should we try to establish some sort of project
policy around this sort of change (ie, removal of backwards-compatibility
support)? "Once it no longer matters for any supported version" sounds
about right to me, but maybe somebody has an argument for thinking about
it differently.
That seems reasonable to me. I don't think we need to mandate that
backwards-compatibility support be removed as soon as it is eligible, but
it can be considered fair game at that point.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Nov 21, 2023 at 9:22 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Nov 21, 2023 at 12:05:36AM -0500, Tom Lane wrote:
No objection here, but should we try to establish some sort of project
policy around this sort of change (ie, removal of backwards-compatibility
support)? "Once it no longer matters for any supported version" sounds
about right to me, but maybe somebody has an argument for thinking about
it differently.That seems reasonable to me. I don't think we need to mandate that
backwards-compatibility support be removed as soon as it is eligible, but
it can be considered fair game at that point.
I think it's easy to miss/enforce a documented policy. IMV, moving
towards pg_attribute_deprecated as Alvaro Herrera said in the other
thread /messages/by-id/202311141920.edtj56saukiv@alvherre.pgsql
can help. Authors then can declare the variables and functions as
deprecated so that the code compilation with
-Wno-deprecated-declarations can help track all such deprecated code.
Having said that, I'm all +1 if the v1 patch proposed in this thread gets in.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Nov 27, 2023 at 04:29:18PM +0530, Bharath Rupireddy wrote:
I think it's easy to miss/enforce a documented policy. IMV, moving
towards pg_attribute_deprecated as Alvaro Herrera said in the other
thread /messages/by-id/202311141920.edtj56saukiv@alvherre.pgsql
can help. Authors then can declare the variables and functions as
deprecated so that the code compilation with
-Wno-deprecated-declarations can help track all such deprecated code.
I'm +1 for adding pg_attribute_deprecated once we have something to use it
for.
Having said that, I'm all +1 if the v1 patch proposed in this thread gets in.
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com