Horton

Thread

Commits

  1. Remove all traces of tuplestore_donestoring() in the C code

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

  1. Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> — 2022-02-16T02:27:58Z

    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

  2. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Michael Paquier <michael@paquier.xyz> — 2022-02-16T02:42:05Z

    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

  3. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> — 2022-02-16T04:25:09Z

    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

  4. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-16T05:23:29Z

    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

  5. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Michael Paquier <michael@paquier.xyz> — 2022-02-16T05:34:00Z

    On Tue, Feb 15, 2022 at 11:23:29PM -0600, Justin Pryzby wrote:
> 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

Thanks for the pointer!  Kasahara-san's patch is doing the job, and I
can see that this does not really influence Melanie's patch, so I
would suggest to move on with this clean up.  I'll go ping the other
thread about that part, first, of course.
--
Michael

  6. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Michael Paquier <michael@paquier.xyz> — 2022-02-17T01:56:30Z

    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

  7. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> — 2022-02-17T02:26:13Z

    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

  8. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-02-17T11:33:55Z

    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

  9. Re: Small and unaffected typo in pg_logical_slot_get_changes_guts()

    Michael Paquier <michael@paquier.xyz> — 2022-02-18T01:28:48Z

    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