Re: logical changeset generation v4

On 2013-01-27 12:28:21 -0500, Steve Singer wrote:
> On 13-01-22 11:30 AM, Andres Freund wrote:
> >Hi,
> >
> >I pushed a new rebased version (the xlogreader commit made it annoying
> >to merge).
> >
> >The main improvements are
> >* way much coherent code internally for intializing logical rep
> >* explicit control over slots
> >* options for logical replication
>
>
> Exactly what is the syntax for using that. My reading your changes to
> repl_gram.y make me think that any of the following should work (but they
> don't).
>
> START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1')
> ERROR: syntax error: unexpected character "("
>
> "START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1' 'val1')
> ERROR: syntax error: unexpected character "("
>
> START_LOGICAL_REPLICATION 'slon1' 0/0 ('opt1','opt2')
> ERROR: syntax error: unexpected character "("

The syntax is right, the grammar (or rather scanner) support is a bit
botched, will push a new version soon.

> I'm also attaching a patch to pg_receivellog that allows you to specify
> these options on the command line. I'm not saying I think that it is
> appropriate to be adding more bells and whistles to the utilities two weeks
> into the CF but I found this useful for testing so I'm sharing it.

The CF is also there to find UI warts and such, so something like this
seems perfectly fine. Even moreso as it doesn't look this will get into
9.3 anyway.

I wanted to add such an option, but I was too lazy^Wbusy to think about
the sematics. Your current syntax doesn't really allow arguments to be
specified in a nice way.
I was thinking of -o name=value and allowing multiple specifications of
-o to build the option string.

Any arguments against that?

> /* Initiate the replication stream at specified location */
> - snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X",
> - slot, (uint32) (startpos >> 32), (uint32) startpos);
> + snprintf(query, sizeof(query), "START_LOGICAL_REPLICATION '%s' %X/%X (%s)",
> + slot, (uint32) (startpos >> 32), (uint32) startpos,plugin_opts);

ISTM that (%s) shouldn't be specified when there are no options, but as
the options need to be pre-escaped anyway, that looks like a non-problem
in a bit more complete implementation.

Greetings,

Andres Freund