Thread: 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

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].

[0] https://postgr.es/m/20200825183002.fkvzxtneijsdgrfv%40alap3.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] https://postgr.es/m/20200825183002.fkvzxtneijsdgrfv%40alap3.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 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 Tue, Nov 14, 2023 at 11:29 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> 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?

Sure. Here it is -
https://www.postgresql.org/message-id/CALj2ACVeO58JM5tK2Qa8QC-%3DkC8sdkJOTd4BFU%3DK8zs4gGYpjQ%40mail.gmail.com.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com