retire MemoryContextResetAndDeleteChildren backwards compatibility macro
I just found myself researching the difference between MemoryContextReset()
and MemoryContextResetAndDeleteChildren(), and it turns out that as of
commit eaa5808 (2015), there is none.
MemoryContextResetAndDeleteChildren() is just a backwards compatibility
macro for MemoryContextReset(). I found this surprising because it sounds
like they do very different things.
Shall we retire this backwards compatibility macro at this point? A search
of https://codesearch.debian.net/ does reveal a few external uses, so we
could alternatively leave it around and just update Postgres to stop using
it, but I don't think it would be too burdensome for extension authors to
fix if we removed it completely.
Patch attached.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
retire_compatibility_macro_v1.patchtext/x-diff; charset=us-asciiDownload+31-34
On Tue, Nov 14, 2023 at 12:30 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:
I just found myself researching the difference between MemoryContextReset()
and MemoryContextResetAndDeleteChildren(), and it turns out that as of
commit eaa5808 (2015), there is none.
MemoryContextResetAndDeleteChildren() is just a backwards compatibility
macro for MemoryContextReset(). I found this surprising because it sounds
like they do very different things.Shall we retire this backwards compatibility macro at this point? A search
of https://codesearch.debian.net/ does reveal a few external uses, so we
could alternatively leave it around and just update Postgres to stop using
it, but I don't think it would be too burdensome for extension authors to
fix if we removed it completely.
+1
Patch attached.
Changes looks pretty much straight forward, but patch failed to apply on the
latest master head(b41b1a7f490) at me.
Regards,
Amul
On Tue, Nov 14, 2023 at 04:25:24PM +0530, Amul Sul wrote:
Changes looks pretty much straight forward, but patch failed to apply on the
latest master head(b41b1a7f490) at me.
Thanks for taking a look. Would you mind sharing the error(s) you are
seeing? The patch applies fine on cfbot and my machine, and check-world
continues to pass.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
On Tue, Nov 14, 2023 at 04:25:24PM +0530, Amul Sul wrote:
Changes looks pretty much straight forward, but patch failed to apply on the
latest master head(b41b1a7f490) at me.
Thanks for taking a look. Would you mind sharing the error(s) you are
seeing? The patch applies fine on cfbot and my machine, and check-world
continues to pass.
It may be a question of the tool used to apply the patch. IME,
"patch" is pretty forgiving, "git am" very much less so.
regards, tom lane
On Tue, Nov 14, 2023 at 10:59:04AM -0500, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
On Tue, Nov 14, 2023 at 04:25:24PM +0530, Amul Sul wrote:
Changes looks pretty much straight forward, but patch failed to apply on the
latest master head(b41b1a7f490) at me.Thanks for taking a look. Would you mind sharing the error(s) you are
seeing? The patch applies fine on cfbot and my machine, and check-world
continues to pass.It may be a question of the tool used to apply the patch. IME,
"patch" is pretty forgiving, "git am" very much less so.
Ah. I just did a 'git diff > file_name' for this one, so you'd indeed need
to use git-apply instead of git-am. (I ordinarily use git-format-patch,
but I sometimes use git-diff for trivial or prototype patches.)
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 2023-Nov-13, Nathan Bossart wrote:
Shall we retire this backwards compatibility macro at this point? A search
of https://codesearch.debian.net/ does reveal a few external uses, so we
could alternatively leave it around and just update Postgres to stop using
it, but I don't think it would be too burdensome for extension authors to
fix if we removed it completely.
Let's leave the macro around and just remove its uses in PGDG-owned
code. Having the macro around hurts nothing, and we can remove it in 15
years or so.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"
On Tue, Nov 14, 2023 at 05:20:16PM +0100, Alvaro Herrera wrote:
Let's leave the macro around and just remove its uses in PGDG-owned
code. Having the macro around hurts nothing, and we can remove it in 15
years or so.
WFM
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2023-Nov-13, Nathan Bossart wrote:
Shall we retire this backwards compatibility macro at this point? A search
of https://codesearch.debian.net/ does reveal a few external uses, so we
could alternatively leave it around and just update Postgres to stop using
it, but I don't think it would be too burdensome for extension authors to
fix if we removed it completely.Let's leave the macro around and just remove its uses in PGDG-owned
code. Having the macro around hurts nothing, and we can remove it in 15
years or so.
Is there a preprocessor symbol that is defined when building Postgres
itself (and extensions in /contrib/), but not third-party extensions (or
vice versa)? If so, the macro could be guarded by that, so that uses
don't accientally sneak back in.
There's also __attribute__((deprecated)) (and and __declspec(deprecated)
for MSVC), but that can AFAIK only be attached to functions and
variables, not macros, so it would have to be changed to a static inline
function.
- ilmari
On Tue, Nov 14, 2023 at 04:36:44PM +0000, Dagfinn Ilmari Manns�ker wrote:
Is there a preprocessor symbol that is defined when building Postgres
itself (and extensions in /contrib/), but not third-party extensions (or
vice versa)? If so, the macro could be guarded by that, so that uses
don't accientally sneak back in.
I'm not aware of anything like that.
There's also __attribute__((deprecated)) (and and __declspec(deprecated)
for MSVC), but that can AFAIK only be attached to functions and
variables, not macros, so it would have to be changed to a static inline
function.
It might be worth introducing pg_attribute_deprecated() in c.h. I'm not
too worried about this particular macro, but it seems handy in general.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Nov 14, 2023 at 11:01:15AM -0600, Nathan Bossart wrote:
On Tue, Nov 14, 2023 at 04:36:44PM +0000, Dagfinn Ilmari Manns�ker wrote:
There's also __attribute__((deprecated)) (and and __declspec(deprecated)
for MSVC), but that can AFAIK only be attached to functions and
variables, not macros, so it would have to be changed to a static inline
function.It might be worth introducing pg_attribute_deprecated() in c.h. I'm not
too worried about this particular macro, but it seems handy in general.
Huh, this was brought up before [0]/messages/by-id/20200825183002.fkvzxtneijsdgrfv@alap3.anarazel.de.
[0]: /messages/by-id/20200825183002.fkvzxtneijsdgrfv@alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
It might be worth introducing pg_attribute_deprecated() in c.h. I'm not
too worried about this particular macro, but it seems handy in general.
Huh, this was brought up before [0].
[0] /messages/by-id/20200825183002.fkvzxtneijsdgrfv@alap3.anarazel.de
FWIW, I think it's fine to just nuke MemoryContextResetAndDeleteChildren.
We ask extension authors to deal with much more significant API changes
than that in every release, and versions where the updated code wouldn't
work are long gone. And, as you say, the existence of that separate from
MemoryContextReset creates confusion, which has nonzero cost in itself.
regards, tom lane
On Tue, Nov 14, 2023 at 9:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Nov-13, Nathan Bossart wrote:
Shall we retire this backwards compatibility macro at this point? A search
of https://codesearch.debian.net/ does reveal a few external uses, so we
could alternatively leave it around and just update Postgres to stop using
it, but I don't think it would be too burdensome for extension authors to
fix if we removed it completely.Let's leave the macro around and just remove its uses in PGDG-owned
code. Having the macro around hurts nothing, and we can remove it in 15
years or so.
FWIW, there are other backward compatibility macros out there like
tuplestore_donestoring which was introduced by commit dd04e95 21 years
ago and SPI_push() and its friends which were made no-ops macros by
commit 1833f1a 7 years ago. Debian code search shows very minimal
usages of the above macros. Can we do away with
tuplestore_donestoring?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Tue, Nov 14, 2023 at 12:10:41PM -0500, Tom Lane wrote:
FWIW, I think it's fine to just nuke MemoryContextResetAndDeleteChildren.
We ask extension authors to deal with much more significant API changes
than that in every release, and versions where the updated code wouldn't
work are long gone. And, as you say, the existence of that separate from
MemoryContextReset creates confusion, which has nonzero cost in itself.
That is my preference as well. Alvaro, AFAICT you are the only vote
against removing it completely. If you feel ѕtrongly about it, I don't
mind going the __attribute__((deprecated)) route, but otherwise, I'd
probably just remove it completely.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Nov 14, 2023 at 10:46:25PM +0530, Bharath Rupireddy wrote:
FWIW, there are other backward compatibility macros out there like
tuplestore_donestoring which was introduced by commit dd04e95 21 years
ago and SPI_push() and its friends which were made no-ops macros by
commit 1833f1a 7 years ago. Debian code search shows very minimal
usages of the above macros. Can we do away with
tuplestore_donestoring?
Can we take these other things to a separate thread?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 2023-Nov-14, Nathan Bossart wrote:
On Tue, Nov 14, 2023 at 12:10:41PM -0500, Tom Lane wrote:
FWIW, I think it's fine to just nuke MemoryContextResetAndDeleteChildren.
We ask extension authors to deal with much more significant API changes
than that in every release, and versions where the updated code wouldn't
work are long gone. And, as you say, the existence of that separate from
MemoryContextReset creates confusion, which has nonzero cost in itself.That is my preference as well. Alvaro, AFAICT you are the only vote
against removing it completely. If you feel ѕtrongly about it,
Oh, I don't. (But I wouldn't mind putting pg_attribute_deprecated to
good use elsewhere ... not that I have any specific examples handy.)
Your S key seems to be doing some funny business.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)
On Tue, Nov 14, 2023 at 08:20:08PM +0100, Alvaro Herrera wrote:
Oh, I don't. (But I wouldn't mind putting pg_attribute_deprecated to
good use elsewhere ... not that I have any specific examples handy.)
Agreed.
Your S key seems to be doing some funny business.
I seem to have accidentally enabled "digraph" in my .vimrc at some point...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Nov 14, 2023 at 9:21 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Tue, Nov 14, 2023 at 04:25:24PM +0530, Amul Sul wrote:
Changes looks pretty much straight forward, but patch failed to apply on
the
latest master head(b41b1a7f490) at me.
Thanks for taking a look. Would you mind sharing the error(s) you are
seeing? The patch applies fine on cfbot and my machine, and check-world
continues to pass.
Nevermind, I usually use git apply or git am, here are those errors:
PG/ - (master) $ git apply ~/Downloads/retire_compatibility_macro_v1.patch
error: patch failed: src/backend/access/brin/brin.c:297
error: src/backend/access/brin/brin.c: patch does not apply
error: patch failed: src/backend/access/gin/ginscan.c:251
error: src/backend/access/gin/ginscan.c: patch does not apply
error: patch failed: src/backend/access/transam/xact.c:1933
error: src/backend/access/transam/xact.c: patch does not apply
error: patch failed: src/backend/commands/analyze.c:583
error: src/backend/commands/analyze.c: patch does not apply
error: patch failed: src/backend/executor/nodeRecursiveunion.c:317
error: src/backend/executor/nodeRecursiveunion.c: patch does not apply
error: patch failed: src/backend/executor/nodeSetOp.c:631
error: src/backend/executor/nodeSetOp.c: patch does not apply
error: patch failed: src/backend/executor/nodeWindowAgg.c:216
error: src/backend/executor/nodeWindowAgg.c: patch does not apply
error: patch failed: src/backend/executor/spi.c:547
error: src/backend/executor/spi.c: patch does not apply
error: patch failed: src/backend/postmaster/autovacuum.c:555
error: src/backend/postmaster/autovacuum.c: patch does not apply
error: patch failed: src/backend/postmaster/bgwriter.c:182
error: src/backend/postmaster/bgwriter.c: patch does not apply
error: patch failed: src/backend/postmaster/checkpointer.c:290
error: src/backend/postmaster/checkpointer.c: patch does not apply
error: patch failed: src/backend/postmaster/walwriter.c:178
error: src/backend/postmaster/walwriter.c: patch does not apply
error: patch failed: src/backend/replication/logical/worker.c:3647
error: src/backend/replication/logical/worker.c: patch does not apply
error: patch failed: src/backend/statistics/extended_stats.c:2237
error: src/backend/statistics/extended_stats.c: patch does not apply
error: patch failed: src/backend/tcop/postgres.c:4457
error: src/backend/tcop/postgres.c: patch does not apply
error: patch failed: src/backend/utils/cache/evtcache.c:91
error: src/backend/utils/cache/evtcache.c: patch does not apply
error: patch failed: src/backend/utils/error/elog.c:1833
error: src/backend/utils/error/elog.c: patch does not apply
error: patch failed: src/include/utils/memutils.h:66
error: src/include/utils/memutils.h: patch does not apply
PG/ - (master) $
Regards,
Amul
On Wed, Nov 15, 2023 at 09:27:18AM +0530, Amul Sul wrote:
Nevermind, I usually use git apply or git am, here are those errors:
PG/ - (master) $ git apply ~/Downloads/retire_compatibility_macro_v1.patch
error: patch failed: src/backend/access/brin/brin.c:297
error: src/backend/access/brin/brin.c: patch does not apply
I wonder if your mail client is modifying the patch. Do you have the same
issue if you download it from the archives?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Nov 15, 2023 at 9:26 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Wed, Nov 15, 2023 at 09:27:18AM +0530, Amul Sul wrote:
Nevermind, I usually use git apply or git am, here are those errors:
PG/ - (master) $ git apply
~/Downloads/retire_compatibility_macro_v1.patch
error: patch failed: src/backend/access/brin/brin.c:297
error: src/backend/access/brin/brin.c: patch does not applyI wonder if your mail client is modifying the patch. Do you have the same
issue if you download it from the archives?
Yes, you are correct. Surprisingly, the archive version applied cleanly.
Gmail is doing something, I usually use web login on chrome browser, I
never
faced such issues with other's patches. Anyway, will try both the versions
next
time for the same kind of issue, sorry for the noise.
Regards,
Amul