Horton

Thread

Commits

  1. Retire MemoryContextResetAndDeleteChildren() macro.

  2. Simplify code by getting rid of SPI_push, SPI_pop, SPI_restore_connection.

  3. Redefine MemoryContextReset() as deleting, not resetting, child contexts.

  4. tuplestore_donestoring() isn't needed anymore, but provide a no-op

  1. retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-13T18:59:50Z

    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

  2. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Amul Sul <sulamul@gmail.com> — 2023-11-14T10:55:24Z

    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

  3. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T15:51:05Z

    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

  4. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-14T15:59:04Z

    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

  5. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T16:05:53Z

    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

  6. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-11-14T16:20:16Z

    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"

  7. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T16:33:39Z

    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

  8. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-11-14T16:36:44Z

    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

  9. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T17:01:15Z

    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

  10. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T17:04:51Z

    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

  11. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-14T17:10:41Z

    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

  12. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-14T17:16:25Z

    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

  13. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T17:59:17Z

    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

  14. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T17:59:53Z

    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

  15. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-11-14T19:20:08Z

    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)

  16. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-14T20:22:52Z

    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

  17. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Amul Sul <sulamul@gmail.com> — 2023-11-15T03:57:18Z

    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

  18. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-15T15:56:00Z

    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

  19. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-15T19:45:14Z

    Committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  20. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Amul Sul <sulamul@gmail.com> — 2023-11-16T06:07:26Z

    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 apply
>
> I 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

  21. Re: retire MemoryContextResetAndDeleteChildren backwards compatibility macro

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-16T13:43:27Z

    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