Greetings, * Michael Paquier (mich...@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
signature.asc
Description: PGP signature
