Horton

Thread

Commits

  1. Add ArchiveOpts to pass options to ArchiveEntry

  2. Remove WITH OIDS support, change oid catalog column visibility.

  1. ArchiveEntry optional arguments refactoring

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-01-16T12:16:40Z

    Hi,

During the discussion in [1] an idea about refactoring ArchiveEntry was
suggested. The reason is that currently this function has significant number of
arguments that are "optional", and every change that has to deal with it
introduces a lot of useless diffs. In the thread, mentioned above, such an
example is tracking current table access method, and I guess "Remove WITH OIDS"
commit 578b229718e is also similar.

Proposed idea is to refactor out all/optional arguments into a separate data
structure, so that adding/removing a new argument wouldn't change that much of
unrelated code. Then for every invocation of ArchiveEntry this structure needs
to be prepared before the call, or as Andres suggested:

    ArchiveEntry((ArchiveArgs){.tablespace = 3,
                               .dumpFn = somefunc,
                               ...});

Another suggestion from Amit is to have an ArchiveEntry() function with limited
number of parameters, and an ArchiveEntryEx() with those extra parameters which
are not needed in usual cases.

I want to prepare a patch for that, and I'm inclined to go with the first
option, but since there are two solutions to choose from, I would love to hear
more opinion about this topic. Any pros/cons we don't see yet?

[1]: https://www.postgresql.org/message-id/flat/20180703070645.wchpu5muyto5n647%40alap3.anarazel.de

  2. Re: ArchiveEntry optional arguments refactoring

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-01-16T12:18:06Z

    > On Wed, Jan 16, 2019 at 1:16 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> Hi,
>
> During the discussion in [1] an idea about refactoring ArchiveEntry was
> suggested. The reason is that currently this function has significant number of
> arguments that are "optional", and every change that has to deal with it
> introduces a lot of useless diffs. In the thread, mentioned above, such an
> example is tracking current table access method, and I guess "Remove WITH OIDS"
> commit 578b229718e is also similar.
>
> Proposed idea is to refactor out all/optional arguments into a separate data
> structure, so that adding/removing a new argument wouldn't change that much of
> unrelated code. Then for every invocation of ArchiveEntry this structure needs
> to be prepared before the call, or as Andres suggested:
>
>     ArchiveEntry((ArchiveArgs){.tablespace = 3,
>                                .dumpFn = somefunc,
>                                ...});
>
> Another suggestion from Amit is to have an ArchiveEntry() function with limited
> number of parameters, and an ArchiveEntryEx() with those extra parameters which
> are not needed in usual cases.
>
> I want to prepare a patch for that, and I'm inclined to go with the first
> option, but since there are two solutions to choose from, I would love to hear
> more opinion about this topic. Any pros/cons we don't see yet?
>
> [1]: https://www.postgresql.org/message-id/flat/20180703070645.wchpu5muyto5n647%40alap3.anarazel.de

[CC Andres and Amit]

  3. Re: ArchiveEntry optional arguments refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-17T15:02:16Z

    On 2019-Jan-16, Dmitry Dolgov wrote:


> Proposed idea is to refactor out all/optional arguments into a separate data
> structure, so that adding/removing a new argument wouldn't change that much of
> unrelated code. Then for every invocation of ArchiveEntry this structure needs
> to be prepared before the call, or as Andres suggested:
> 
>     ArchiveEntry((ArchiveArgs){.tablespace = 3,
>                                .dumpFn = somefunc,
>                                ...});

Prepping the struct before the call would be our natural style, I think.
This one where the struct is embedded in the function call does not look
*too* horrible, but I'm curious as to what does pgindent do with it.

> Another suggestion from Amit is to have an ArchiveEntry() function with limited
> number of parameters, and an ArchiveEntryEx() with those extra parameters which
> are not needed in usual cases.

Is there real savings to be had by doing this?  What would be the
arguments to each function?  Off-hand, I'm not liking this idea too
much.  But maybe we can combine both ideas and have one "normal"
function with only the most common args, and create ArchiveEntryExtended
to use the struct as proposed by Andres.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: ArchiveEntry optional arguments refactoring

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T15:23:39Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-16, Dmitry Dolgov wrote:
>> ArchiveEntry((ArchiveArgs){.tablespace = 3,
>> .dumpFn = somefunc,
>> ...});

> Is there real savings to be had by doing this?  What would be the
> arguments to each function?  Off-hand, I'm not liking this idea too
> much.

I'm not either.  What this looks like it will mainly do is create
a back-patching barrier, with little if any readability improvement.

I don't buy the argument that this would move the goalposts in terms
of how much work it is to add a new argument.  You'd still end up
touching every call site.

			regards, tom lane

  5. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-17T17:29:04Z

    On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Jan-16, Dmitry Dolgov wrote:
> >> ArchiveEntry((ArchiveArgs){.tablespace = 3,
> >> .dumpFn = somefunc,
> >> ...});
>
> > Is there real savings to be had by doing this?  What would be the
> > arguments to each function?  Off-hand, I'm not liking this idea too
> > much.
>
> I'm not either.  What this looks like it will mainly do is create
> a back-patching barrier, with little if any readability improvement.

I don't really buy this. We've whacked around the arguments to
ArchiveEntry() repeatedly over the last few releases, so there's already
a hindrance to backpatching. And given the current setup we've to whack
around all 70+ callsites whenever a single argument is added. With the
setup I'd suggested you don't, because the designated initializer syntax
allows you to omit items that ought to be zero-initialized.

And given the number of arguments to ArchiveEntry() having a name for
each argument would help for readability too. It's currently not exactly
obvious what is an argument for what:
	ArchiveEntry(AH, nilCatalogId, createDumpId(),
				 "ENCODING", NULL, NULL, "",
				 "ENCODING", SECTION_PRE_DATA,
				 qry->data, "", NULL,
				 NULL, 0,
				 NULL, NULL);

If you compare that with

	ArchiveEntry(AH,
                 (ArchiveEntry){.catalogId = nilCatalogId,
                                .dumpId = createDumpId(),
                                .tag = "ENCODING",
                                .desc = "ENCODING",
                                .section = SECTION_PRE_DATA,
                                .defn = qry->data});

it's definitely easier to see what argument is what.


> I don't buy the argument that this would move the goalposts in terms
> of how much work it is to add a new argument.  You'd still end up
> touching every call site.

Why? A lot of arguments that'd be potentially added or removed would not
be set by each callsites.

If you e.g. look at

you can see that a lot of changes where like
        ArchiveEntry(fout, nilCatalogId, createDumpId(),
                     "pg_largeobject", NULL, NULL, "",
-                    false, "pg_largeobject", SECTION_PRE_DATA,
+                    "pg_largeobject", SECTION_PRE_DATA,
                     loOutQry->data, "", NULL,
                     NULL, 0,
                     NULL, NULL);

i.e. just removing a 'false' argument. In like 70+ callsites. With the
above scheme, we'd instead just have removed a single .withoids = true,
from dumpTableSchema()'s ArchiveEntry() call.

Greetings,

Andres Freund

  6. Re: ArchiveEntry optional arguments refactoring

    Stephen Frost <sfrost@snowman.net> — 2019-01-17T18:20:30Z

    Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > On 2019-Jan-16, Dmitry Dolgov wrote:
> > >> ArchiveEntry((ArchiveArgs){.tablespace = 3,
> > >> .dumpFn = somefunc,
> > >> ...});
> >
> > > Is there real savings to be had by doing this?  What would be the
> > > arguments to each function?  Off-hand, I'm not liking this idea too
> > > much.
> >
> > I'm not either.  What this looks like it will mainly do is create
> > a back-patching barrier, with little if any readability improvement.
> 
> I don't really buy this. We've whacked around the arguments to
> ArchiveEntry() repeatedly over the last few releases, so there's already
> a hindrance to backpatching. And given the current setup we've to whack
> around all 70+ callsites whenever a single argument is added. With the
> setup I'd suggested you don't, because the designated initializer syntax
> allows you to omit items that ought to be zero-initialized.
> 
> And given the number of arguments to ArchiveEntry() having a name for
> each argument would help for readability too. It's currently not exactly
> obvious what is an argument for what:
> 	ArchiveEntry(AH, nilCatalogId, createDumpId(),
> 				 "ENCODING", NULL, NULL, "",
> 				 "ENCODING", SECTION_PRE_DATA,
> 				 qry->data, "", NULL,
> 				 NULL, 0,
> 				 NULL, NULL);
> 
> If you compare that with
> 
> 	ArchiveEntry(AH,
>                  (ArchiveEntry){.catalogId = nilCatalogId,
>                                 .dumpId = createDumpId(),
>                                 .tag = "ENCODING",
>                                 .desc = "ENCODING",
>                                 .section = SECTION_PRE_DATA,
>                                 .defn = qry->data});
> 
> it's definitely easier to see what argument is what.

+1.  I was on the fence about this approach when David started using it
in pgBackRest but I've come to find that it's actually pretty nice and
being able to omit things which should be zero/default is very nice.  I
feel like it's quite similar to what we do in other places too- just
look for things like:

utils/adt/jsonfuncs.c:600

        sem = palloc0(sizeof(JsonSemAction));

...

        sem->semstate = (void *) state;
        sem->array_start = okeys_array_start;
        sem->scalar = okeys_scalar;
        sem->object_field_start = okeys_object_field_start;
        /* remainder are all NULL, courtesy of palloc0 above */

        pg_parse_json(lex, sem);

...

        pfree(sem);

> > I don't buy the argument that this would move the goalposts in terms
> > of how much work it is to add a new argument.  You'd still end up
> > touching every call site.
> 
> Why? A lot of arguments that'd be potentially added or removed would not
> be set by each callsites.
> 
> If you e.g. look at
> 
> you can see that a lot of changes where like
>         ArchiveEntry(fout, nilCatalogId, createDumpId(),
>                      "pg_largeobject", NULL, NULL, "",
> -                    false, "pg_largeobject", SECTION_PRE_DATA,
> +                    "pg_largeobject", SECTION_PRE_DATA,
>                      loOutQry->data, "", NULL,
>                      NULL, 0,
>                      NULL, NULL);
> 
> i.e. just removing a 'false' argument. In like 70+ callsites. With the
> above scheme, we'd instead just have removed a single .withoids = true,
> from dumpTableSchema()'s ArchiveEntry() call.

Agreed.  Using this approach in more places, when appropriate and
sensible, seems like a good direction to go in.  To be clear, I don't
think we should go rewrite pieces of code just for the sake of it as
that would make back-patching more difficult, but when we're making
changes anyway, or where it wouldn't really change the landscape for
back-patching, then it seems like a good change.

Thanks!

Stephen

  7. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-17T18:26:32Z

    Hi,

On 2019-01-17 09:29:04 -0800, Andres Freund wrote:
> On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> > I don't buy the argument that this would move the goalposts in terms
> > of how much work it is to add a new argument.  You'd still end up
> > touching every call site.
> 
> Why? A lot of arguments that'd be potentially added or removed would not
> be set by each callsites.
> 
> If you e.g. look at
> 
> you can see that a lot of changes where like
>         ArchiveEntry(fout, nilCatalogId, createDumpId(),
>                      "pg_largeobject", NULL, NULL, "",
> -                    false, "pg_largeobject", SECTION_PRE_DATA,
> +                    "pg_largeobject", SECTION_PRE_DATA,
>                      loOutQry->data, "", NULL,
>                      NULL, 0,
>                      NULL, NULL);
> 
> i.e. just removing a 'false' argument. In like 70+ callsites. With the
> above scheme, we'd instead just have removed a single .withoids = true,
> from dumpTableSchema()'s ArchiveEntry() call.

the "at" I was trying to reference above is
578b229718e8f15fa779e20f086c4b6bb3776106 / the WITH OID removal, and
therein specifically the pg_dump changes.

Greetings,

Andres Freund

  8. Re: ArchiveEntry optional arguments refactoring

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2019-01-18T04:36:40Z

    On Wed, 16 Jan 2019 at 17:45, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> Hi,
>
> During the discussion in [1] an idea about refactoring ArchiveEntry was
> suggested. The reason is that currently this function has significant number of
> arguments that are "optional", and every change that has to deal with it
> introduces a lot of useless diffs. In the thread, mentioned above, such an
> example is tracking current table access method, and I guess "Remove WITH OIDS"
> commit 578b229718e is also similar.
>
> Proposed idea is to refactor out all/optional arguments into a separate data
> structure, so that adding/removing a new argument wouldn't change that much of
> unrelated code. Then for every invocation of ArchiveEntry this structure needs
> to be prepared before the call, or as Andres suggested:
>
>     ArchiveEntry((ArchiveArgs){.tablespace = 3,
>                                .dumpFn = somefunc,
>                                ...});

I didn't know we could do it this way. I thought we would have to
declare a variable and have to initialize fields with non-const values
separately. This looks nice. We could even initialize fields with
non-const values. +1 from me.

I think, we could use the same TocEntry structure as parameter, rather
than a new structure. Most of the arguments already resemble fields of
this structure. Also, we could pass pointer to that structure :

     ArchiveEntry( &(TocEntry){.tablespace = 3,
                               .dumpFn = somefunc,
                               ...});



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

  9. Re: ArchiveEntry optional arguments refactoring

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-01-23T15:12:15Z

    > On 2019-01-17 09:29:04 -0800, Andres Freund wrote:
> On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> > I don't buy the argument that this would move the goalposts in terms
> > of how much work it is to add a new argument.  You'd still end up
> > touching every call site.
>
> Why? A lot of arguments that'd be potentially added or removed would not
> be set by each callsites.
>
> If you e.g. look at
>
> you can see that a lot of changes where like
>         ArchiveEntry(fout, nilCatalogId, createDumpId(),
>                      "pg_largeobject", NULL, NULL, "",
> -                    false, "pg_largeobject", SECTION_PRE_DATA,
> +                    "pg_largeobject", SECTION_PRE_DATA,
>                      loOutQry->data, "", NULL,
>                      NULL, 0,
>                      NULL, NULL);
>
> i.e. just removing a 'false' argument. In like 70+ callsites. With the
> above scheme, we'd instead just have removed a single .withoids = true,
> from dumpTableSchema()'s ArchiveEntry() call.

To make this discussion a bit more specific, I've created a patch of how it can
look like. All the arguments, except Archive, CatalogId and DumpId I've moved
into the ArchiveOpts structure. Not all of them could be empty before, but
anyway it seems better for consistency and readability. Some of the arguments
had empty string as a default value, I haven't changed anything here yet
(although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).

As Andres mentioned above, for 578b229718e / the WITH OID removal and pg_dump
modification from pluggable storage thread, this patch reduces number of
changes, related to ArchiveEntry, from 70+ to just one.

  10. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T16:47:34Z

    Hi,

On 2019-01-23 16:12:15 +0100, Dmitry Dolgov wrote:
> To make this discussion a bit more specific, I've created a patch of how it can
> look like.

Thanks.

> All the arguments, except Archive, CatalogId and DumpId I've moved
> into the ArchiveOpts structure. Not all of them could be empty before, but
> anyway it seems better for consistency and readability. Some of the arguments
> had empty string as a default value, I haven't changed anything here yet
> (although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).

Probably worth changing at the same time, if we decide to go for it.

To me this does look like it'd be more maintainable going forward.

Greetings,

Andres Freund

  11. Re: ArchiveEntry optional arguments refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-23T16:58:07Z

    Hello

On 2019-Jan-23, Andres Freund wrote:

> > All the arguments, except Archive, CatalogId and DumpId I've moved
> > into the ArchiveOpts structure. Not all of them could be empty before, but
> > anyway it seems better for consistency and readability. Some of the arguments
> > had empty string as a default value, I haven't changed anything here yet
> > (although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).
> 
> Probably worth changing at the same time, if we decide to go for it.
> 
> To me this does look like it'd be more maintainable going forward.

It does.  How does pgindent behave with it?

I'd use ArchiveEntryOpts as struct name; ArchiveOpts sounds wrong. Also,
the struct members could use better names -- "defn" for example could
perhaps be "createStmt" (to match dropStmt/copyStmt), and expand "desc"
to "description".

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: ArchiveEntry optional arguments refactoring

    Chapman Flack <chap@anastigmatix.net> — 2019-01-23T17:05:10Z

    On 1/23/19 10:12 AM, Dmitry Dolgov wrote:
> To make this discussion a bit more specific, I've created a patch of how
> it can look like.
A little bit of vararg-macro action can make such a design look
even tidier, cf. [1].

Or are compilers without vararg macros still in the supported mix?

-Chap



[1] https://github.com/NetBSD/src/blob/trunk/sys/sys/midiio.h#L709

The macros in [1] are not defined to create a function call, but only
the argument structure because there might be several functions to pass
it to, so a call would be written like func(&SEQ_MK_CHN(NOTEON, ...)).

In ArchiveEntry's case, if there's only one function involved, there'd
be no reason not to have a macro produce the whole call.

  13. Re: ArchiveEntry optional arguments refactoring

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-23T17:05:43Z

    Chapman Flack <chap@anastigmatix.net> writes:
> Or are compilers without vararg macros still in the supported mix?

No.

			regards, tom lane

  14. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T17:07:01Z

    Hi,

On 2019-01-23 13:58:07 -0300, Alvaro Herrera wrote:
> I'd use ArchiveEntryOpts as struct name; ArchiveOpts sounds wrong.

Brevity would be of some advantage IMO, because it'll probably determine
how pgindent indents the arguments, because the struct name will be in
the arguments.


> Also, the struct members could use better names -- "defn" for example
> could perhaps be "createStmt" (to match dropStmt/copyStmt), and expand
> "desc" to "description".

True.

Greetings,

Andres Freund

  15. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T17:10:12Z

    Hi,

On 2019-01-23 12:05:10 -0500, Chapman Flack wrote:
> On 1/23/19 10:12 AM, Dmitry Dolgov wrote:
> > To make this discussion a bit more specific, I've created a patch of how
> > it can look like.

> A little bit of vararg-macro action can make such a design look
> even tidier, cf. [1].
> [1] https://github.com/NetBSD/src/blob/trunk/sys/sys/midiio.h#L709
> 
> The macros in [1] are not defined to create a function call, but only
> the argument structure because there might be several functions to pass
> it to, so a call would be written like func(&SEQ_MK_CHN(NOTEON, ...)).
> 
> In ArchiveEntry's case, if there's only one function involved, there'd
> be no reason not to have a macro produce the whole call.

I'm not really seeing this being more than obfuscation in this case. The
only point of the macro is to set the .tag and .op elements to something
without adding redundancies due to the struct name. Which we'd not have.

Greetings,

Andres Freund

  16. Re: ArchiveEntry optional arguments refactoring

    Chapman Flack <chap@anastigmatix.net> — 2019-01-23T17:22:23Z

    On 1/23/19 12:10 PM, Andres Freund wrote:
> On 2019-01-23 12:05:10 -0500, Chapman Flack wrote:
>> [1] https://github.com/NetBSD/src/blob/trunk/sys/sys/midiio.h#L709

> I'm not really seeing this being more than obfuscation in this case. The
> only point of the macro is to set the .tag and .op elements to something
> without adding redundancies due to the struct name. Which we'd not have.

Granted, that example is more elaborate than this case, but writing


ArchiveEntry(fout, dbCatId, dbDumpId, .tag = datname, .owner = dba,
             .desc = "DATABASE", .section = SECTION_PRE_DATA,
             .defn = creaQry->data, .dropStmt = delQry->data);

instead of

ArchiveEntry(fout, dbCatId, dbDumpId, &(ArchiveOpts){.tag = datname,
             .owner = dba, .desc = "DATABASE",
             .section = SECTION_PRE_DATA, .defn = creaQry->data,
             .dropStmt = delQry->data});

would be easy, and still save a bit of visual noise.

Regards,
-Chap

  17. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T17:23:28Z

    On 2019-01-23 13:58:07 -0300, Alvaro Herrera wrote:
> Hello
> 
> On 2019-Jan-23, Andres Freund wrote:
> 
> > > All the arguments, except Archive, CatalogId and DumpId I've moved
> > > into the ArchiveOpts structure. Not all of them could be empty before, but
> > > anyway it seems better for consistency and readability. Some of the arguments
> > > had empty string as a default value, I haven't changed anything here yet
> > > (although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).
> > 
> > Probably worth changing at the same time, if we decide to go for it.
> > 
> > To me this does look like it'd be more maintainable going forward.
> 
> It does.  How does pgindent behave with it?

It craps out:
Error@3649: Unbalanced parens
Warning@3657: Extra )

But that can be worked around with something like

        te = ArchiveEntry(fout, tdinfo->dobj.catId, tdinfo->dobj.dumpId,
                          ARCHIVE_ARGS(.tag = tbinfo->dobj.name,
                                       .namespace = tbinfo->dobj.namespace->dobj.name,
                                       .owner = tbinfo->rolname,
                                       .desc = "TABLE DATA",
                                       .section = SECTION_DATA,
                                       .copyStmt = copyStmt,
                                       .deps = &(tbinfo->dobj.dumpId),
                                       .nDeps = 1,
                                       .dumpFn = dumpFn,
                                       .dumpArg = tdinfo,
                                       ));
which looks mildly simpler too.

Greetings,

Andres Freund

  18. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T17:25:09Z

    On 2019-01-23 12:22:23 -0500, Chapman Flack wrote:
> On 1/23/19 12:10 PM, Andres Freund wrote:
> > On 2019-01-23 12:05:10 -0500, Chapman Flack wrote:
> >> [1] https://github.com/NetBSD/src/blob/trunk/sys/sys/midiio.h#L709
> 
> > I'm not really seeing this being more than obfuscation in this case. The
> > only point of the macro is to set the .tag and .op elements to something
> > without adding redundancies due to the struct name. Which we'd not have.
> 
> Granted, that example is more elaborate than this case, but writing
> 
> 
> ArchiveEntry(fout, dbCatId, dbDumpId, .tag = datname, .owner = dba,
>              .desc = "DATABASE", .section = SECTION_PRE_DATA,
>              .defn = creaQry->data, .dropStmt = delQry->data);
> 
> instead of
> 
> ArchiveEntry(fout, dbCatId, dbDumpId, &(ArchiveOpts){.tag = datname,
>              .owner = dba, .desc = "DATABASE",
>              .section = SECTION_PRE_DATA, .defn = creaQry->data,
>              .dropStmt = delQry->data});
> 
> would be easy, and still save a bit of visual noise.

IDK, it'd be harder to parse correctly as a C programmer though. I'm up
with a wrapper macro like
#define ARCHIVE_ARGS(...) &(ArchiveOpts){__VA_ARGS__}
but weirdly mixing struct arguments and normal function arguments seems
quite confusing.

Greetings,

Andres Freund

  19. Re: ArchiveEntry optional arguments refactoring

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-23T17:32:06Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-01-23 13:58:07 -0300, Alvaro Herrera wrote:
>> It does.  How does pgindent behave with it?

> It craps out:
> Error@3649: Unbalanced parens
> Warning@3657: Extra )

> But that can be worked around with something like

>         te = ArchiveEntry(fout, tdinfo->dobj.catId, tdinfo->dobj.dumpId,
>                           ARCHIVE_ARGS(.tag = tbinfo->dobj.name,
>                                        .namespace = tbinfo->dobj.namespace->dobj.name,
>                                        .owner = tbinfo->rolname,
>                                        .desc = "TABLE DATA",
>                                        .section = SECTION_DATA,
>                                        .copyStmt = copyStmt,
>                                        .deps = &(tbinfo->dobj.dumpId),
>                                        .nDeps = 1,
>                                        .dumpFn = dumpFn,
>                                        .dumpArg = tdinfo,
>                                        ));
> which looks mildly simpler too.

That looks fairly reasonable from here, but I'd suggest
ARCHIVE_OPTS rather than ARCHIVE_ARGS.

Can we omit the initial dots if we use a wrapper macro?  Would it be
a good idea to do so (I'm not really sure)?

			regards, tom lane

  20. Re: ArchiveEntry optional arguments refactoring

    Andres Freund <andres@anarazel.de> — 2019-01-23T17:36:13Z

    On 2019-01-23 12:32:06 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-01-23 13:58:07 -0300, Alvaro Herrera wrote:
> >> It does.  How does pgindent behave with it?
> 
> > It craps out:
> > Error@3649: Unbalanced parens
> > Warning@3657: Extra )
> 
> > But that can be worked around with something like
> 
> >         te = ArchiveEntry(fout, tdinfo->dobj.catId, tdinfo->dobj.dumpId,
> >                           ARCHIVE_ARGS(.tag = tbinfo->dobj.name,
> >                                        .namespace = tbinfo->dobj.namespace->dobj.name,
> >                                        .owner = tbinfo->rolname,
> >                                        .desc = "TABLE DATA",
> >                                        .section = SECTION_DATA,
> >                                        .copyStmt = copyStmt,
> >                                        .deps = &(tbinfo->dobj.dumpId),
> >                                        .nDeps = 1,
> >                                        .dumpFn = dumpFn,
> >                                        .dumpArg = tdinfo,
> >                                        ));
> > which looks mildly simpler too.
> 
> That looks fairly reasonable from here, but I'd suggest
> ARCHIVE_OPTS rather than ARCHIVE_ARGS.

WFM. Seems quite possible that we'd grow a few more of these over time,
so establishing some common naming seems good.

Btw, do you have an opionion on keeping catId / dumpId outside/inside
the argument struct?


> Can we omit the initial dots if we use a wrapper macro?  Would it be
> a good idea to do so (I'm not really sure)?

Not easily, if at all, I think. We'd have to do a fair bit of weird
macro magic (and then still end up with limitations) to "process" each
argument individually.  And even if it were easy, I don't think it's
particularly advantageous.

Greetings,

Andres Freund

  21. Re: ArchiveEntry optional arguments refactoring

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-23T18:33:09Z

    Andres Freund <andres@anarazel.de> writes:
> Btw, do you have an opionion on keeping catId / dumpId outside/inside
> the argument struct?

I'd go for outside, since they're not optional.  Not dead set on that
though.

			regards, tom lane

  22. Re: ArchiveEntry optional arguments refactoring

    Chapman Flack <chap@anastigmatix.net> — 2019-01-23T18:36:36Z

    On 1/23/19 12:25 PM, Andres Freund wrote:
> On 2019-01-23 12:22:23 -0500, Chapman Flack wrote:

>> ArchiveEntry(fout, dbCatId, dbDumpId, .tag = datname, .owner = dba,
>>              .desc = "DATABASE", .section = SECTION_PRE_DATA,
>>              .defn = creaQry->data, .dropStmt = delQry->data);

> IDK, it'd be harder to parse correctly as a C programmer though. ...
> weirdly mixing struct arguments and normal function arguments seems
> quite confusing.

Hmm, I guess the rubric I think with goes something like "is a C
programmer who encounters this in a source file for the first time
likely to guess wrong about what it means?", and in the case above,
I can scarcely imagine it.

ISTM that these days, many people are familiar with several languages
that allow a few mandatory, positional parameters followed by optional
named ones, and so a likely reaction would be "hey look, somebody used
a macro here to make C look more like <insert other language I know>."

On 1/23/19 12:32 PM, Tom Lane wrote:
> Can we omit the initial dots if we use a wrapper macro?

That, I think, is hard.

Getting to the form above is downright easy; making the dots go away,
even if achievable, seems way further down the path of diminishing
returns.

Regards,
-Chap

  23. Re: ArchiveEntry optional arguments refactoring

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-01-24T12:12:40Z

    Here is another version, where I accumulated all the suggestions:

* Use NULL as a default value where it was an empty string before (this
  required few minor changes for some part of the code outside ArchiveEntry)

* Rename defn, descr to createStmt, description

* Use a macro to avoid pgindent errors

About the last one. I'm also inclined to use the simpler version of
ARCHIVE_OPTS macro, mostly because the difference between "optional" and
"positional" arguments in the alternative proposal is not that visible. So

> mixing struct arguments and normal function arguments seems
> quite confusing

could probably affect not only readability, but also would be bit more
problematic for updating this code (which was the goal in the first place).

  24. Re: ArchiveEntry optional arguments refactoring

    Daniel Gustafsson <daniel@yesql.se> — 2019-02-01T10:05:34Z

    > On 24 Jan 2019, at 13:12, Dmitry Dolgov <9erthalion6@gmail.com> wrote: 

> Here is another version, where I accumulated all the suggestions:

Nothing sticks out during review and AFAICT all comments have been addressed.
Everything works as expected during (light) testing between master and an older
version.

+1 on committing this, having spent a lot of time in this code I really
appreciate the improved readability.

cheers ./daniel

  25. Re: ArchiveEntry optional arguments refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-01T11:33:49Z

    pgindent didn't like your layout with two-space indents for the struct
members :-(  I thought it was nice, but oh well.  This means we can do
away with the newline at each callsite, and I didn't like the trailing
comma (and I have vague recollections that some old compilers might
complain about them too, though maybe we retired them already.)

> * Use NULL as a default value where it was an empty string before (this
>   required few minor changes for some part of the code outside ArchiveEntry)

Ah, so this is why you changed replace_line_endings.  So the comment on
that function now is wrong -- it fails to indicate that it returns a
malloc'ed "" on NULL input.  But about half the callers want to have a
malloc'ed "-" on NULL input ... I think it'd make the code a little bit
simpler if we did that in replace_line_endings itself, maybe add a
"want_dash" bool argument, so this code

		if (!ropt->noOwner)
			sanitized_owner = replace_line_endings(te->owner);
		else
			sanitized_owner = pg_strdup("-");

can become
		sanitized_owner = replace_line_endings(te->owner, true);

I don't quite understand why the comments about line sanitization were
added in the callsites rather than in replace_line_endings itself.  I
would rename the function to sanitize_line() and put those comments
there (removing them from the callsites), then the new argument I
suggest would not be completely out of place.

What do you think?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: ArchiveEntry optional arguments refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-01T11:43:23Z

    On 2019-Feb-01, Alvaro Herrera wrote:

> ... so this code
> 
> 		if (!ropt->noOwner)
> 			sanitized_owner = replace_line_endings(te->owner);
> 		else
> 			sanitized_owner = pg_strdup("-");
> 
> can become
> 		sanitized_owner = replace_line_endings(te->owner, true);

Sorry, there's a silly bug here because I picked the wrong example to
hand-type.  The proposed pattern works fine for the schema cases, not
for this owner case.  The owner case is correctly handled (AFAICT) in
the patch I posted.  (Also for some reason I decided to go with "hyphen"
instead of "dash" in the argument name.  Not sure if anybody cares
strongly about using the right terminology there (I don't know which it
is).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  27. Re: ArchiveEntry optional arguments refactoring

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-02-01T14:25:50Z

    > On Fri, Feb 1, 2019 at 12:33 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> pgindent didn't like your layout with two-space indents for the struct
> members :-(  I thought it was nice, but oh well.  This means we can do
> away with the newline at each callsite, and I didn't like the trailing
> comma (and I have vague recollections that some old compilers might
> complain about them too, though maybe we retired them already.)

Oh, ok. In fact I did this almost automatically without thinking too much (a
formatting habit from other languages), so if pgindent doesn't like it, then
fine.

> > * Use NULL as a default value where it was an empty string before (this
> >   required few minor changes for some part of the code outside ArchiveEntry)
>
> I would rename the function to sanitize_line() and put those comments there
> (removing them from the callsites), then the new argument I suggest would not
> be completely out of place.

Yes, sounds pretty reasonable for me.

> (Also for some reason I decided to go with "hyphen" instead of "dash" in the
> argument name.  Not sure if anybody cares strongly about using the right
> terminology there (I don't know which it is).

Just out of curiosity I did some search and could find few examples of using
both "dash" and "hyphen" across the code, but I guess indeed it doesn't really
matter.

  28. Re: ArchiveEntry optional arguments refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-01T14:30:58Z

    On 2019-Feb-01, Dmitry Dolgov wrote:

> > On Fri, Feb 1, 2019 at 12:33 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> > > * Use NULL as a default value where it was an empty string before (this
> > >   required few minor changes for some part of the code outside ArchiveEntry)
> >
> > I would rename the function to sanitize_line() and put those comments there
> > (removing them from the callsites), then the new argument I suggest would not
> > be completely out of place.
> 
> Yes, sounds pretty reasonable for me.

Thanks for looking -- pushed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services