Обсуждение: Small and unaffected typo in pg_logical_slot_get_changes_guts()

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

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

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

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.
https://www.postgresql.org/message-id/20220106005748.GT14051%40telsasoft.com

-- 
Justin

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]: https://www.postgresql.org/message-id/20220106005748.GT14051%40telsasoft.com
--
Michael

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]: https://www.postgresql.org/message-id/20220106005748.GT14051%40telsasoft.com
Thanks !

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

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

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