Re: Using base backup exclusion filters to reduce data transferredwith pg_rewind - Mailing list pgsql-hackers

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Fri, Mar 23, 2018 at 01:38:38AM +0900, Fujii Masao wrote:
> > Personally it looks very intrusive, so I'm feeling inclined to push
> > the changes without that refactoring.

I've been reading over the first couple of posted patches and mulling
over the changes proposed.  They certainly touch a lot of places but
they're pretty straight-forward changes and so I'm not really sure I'd
call them all that intrusive.

I don't completely buy off on the argument that having these #define's
would make it easier for forks (we've had quite a few folks fork PG, but
how many of them have actually changed "base"?) and I'm on the fence
about if these will make our lives simpler down the road when it comes
to changing the directory names (if we changed "pg_multixact/members" to
be "pg_multixact/processes" or some such, I imagine we'd also go through
and change PG_MULTIXACT_MEMBERS_DIR to be PG_MULTIXACT_PROCESSES_DIR
anyway, so this doesn't result in much improvement there), but as it
relates to new tool development and such, there's some value here
because the compiler is going to complain if you say
PG_COMMIT_TX_DIR and there is no such #define, whereas a similar mistake
with a string literal of "pg_commit_tx" might end up getting missed and
that would be unfortunate.

Therefore, on the whole, I'm +1 on these changes, but I'd argue a bit
about some of the choices made:

- Let's have them all be PG_WHATEVER_DIR/FILE
- Use WAL instead of XLOG (I thought we agreed new code would..?)
- Remove extraneous #define's (most of these were, but
  DIRECTORY_LOCK_FILE was kept..?  Let's use PG_POSTMASTER_PID_FILE
  throughout instead, with a comment that it's also used as a lock
  file).

Might be nice to go back and modify other pre-existing #define's to use
that form also, but perhaps not that big of a deal.  I would have that
be independent from this, in any case.

> Okay.  Just moving the list of items from basebackup.c to a dedicated
> header is not sufficient though as things like RELCACHE_INIT_FILENAME as
> declared in headers which are backend-only.  The same applies to
> LOG_METAINFO_DATAFILE_TMP, PG_AUTOCONF_FILENAME, PG_STAT_TMP_DIR and
> PG_DYNSHMEM_DIR.
>
> BACKUP_LABEL_FILE and TABLESPACE_MAP can be included though via xlog.h.
> So what are you looking for?  I see a couple of options:
> 1) The inclusive refactoring, which you are discarding.
> 2) A dedicated header, but some of the now-not-hardcoded values will
> need to be so.  That's the list I am giving above.
> 3) A copy of the list from basebackup.c to src/bin/pg_rewind/.
>
> I would guess that you are looking for 2), but I am not sure if you
> imply that 3) would be acceptable or not.

Yeah, neither 2 or 3 really appeals to me.  Option 1 does touch a number
of places but in a pretty straight-forward way- and if there's a typo
there, the compiler is likely to complain, so it seems like the risk is
relatively low.

Thanks!

Stephen