Re: refactoring basebackup.c

On Tue, Sep 14, 2021 at 11:30 AM Sergei Kornilov <sk(at)zsrv(dot)org> wrote:
> I found that in 0001 you propose to rename few options. Probably we could rename another option for clarify? I think FAST (it's about some bw limits?) and WAIT (wait for what? checkpoint?) option names are confusing.
> Could we replace FAST with "CHECKPOINT [fast|spread]" and WAIT to WAIT_WAL_ARCHIVED? I think such names would be more descriptive.

I think CHECKPOINT { 'spread' | 'fast' } is probably a good idea; the
options logic for pg_basebackup uses the same convention, and if
somebody ever wanted to introduce a third kind of checkpoint, it would
be a lot easier if you could just make pg_basebackup -cbanana send
CHECKPOINT 'banana' to the server. I don't think renaming WAIT ->
WAIT_WAL_ARCHIVED has much value. The replication grammar isn't really
intended to be consumed directly by end-users, and it's also not clear
that WAIT_WAL_ARCHIVED would attract more support than any of 5 or 10
other possible variants. I'd rather leave it alone.

> - if (PQserverVersion(conn) >= 100000)
> - /* pg_recvlogical doesn't use an exported snapshot, so suppress */
> - appendPQExpBufferStr(query, " NOEXPORT_SNAPSHOT");
> + /* pg_recvlogical doesn't use an exported snapshot, so suppress */
> + if (use_new_option_syntax)
> + AppendStringCommandOption(query, use_new_option_syntax,
> + "SNAPSHOT", "nothing");
> + else
> + AppendPlainCommandOption(query, use_new_option_syntax,
> + "NOEXPORT_SNAPSHOT");
>
> In 0002, it looks like condition for 9.x releases was lost?

Good catch, thanks.

I'll post an updated version of these two patches on the thread
dedicated to those two patches, which can be found at
http://postgr.es/m/CA+Tgmob2cbCPNbqGoixp0J6aib0p00XZerswGZwx-5G=0M+BMA@mail.gmail.com

> Also my gcc version 8.3.0 is not happy with v5-0007-Support-base-backup-targets.patch and produces:
>
> basebackup.c: In function ‘parse_basebackup_options’:
> basebackup.c:970:7: error: ‘target_str’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> errmsg("target '%s' does not accept a target detail",
> ^~~~~~

OK, I'll fix that. Thanks.

--
Robert Haas
EDB: http://www.enterprisedb.com