Re: fix tablespace handling in pg_combinebackup

On Thu, Apr 18, 2024 at 1:45 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> I was really just remarking on this from the angle of a test writer. I know
> that our interfaces around this suck...
>
> For tests, do we really need to set anything on a per-tablespace basis? Can't
> we instead just reparent all of them to a new directory?

I don't know what you mean by this.

> > I wonder if we (as a project) would consider a patch that redesigned
> > this whole mechanism. Produce ${TABLESPACE_NAME}.tar in tar-format,
> > instead of ${OID}.tar. In directory-format, relocate via
> > -T${TABLESPACE_NAME}=${DIR} instead of -T${SERVERDIR}=${DIR}. That
> > would be a significant compatibility break, and you'd somehow need to
> > solve the problem of what to put in the tablespace_map file, which
> > requires OIDs. But it seems like if you could finesse that issue in
> > some elegant way, the result would just be a heck of a lot more usable
> > than what we have today.
>
> For some things that'd definitely be nicer - but not sure it work well for
> everything. Consider cases where you actually have external directories on
> different disks, and you want to restore a backup after some data loss. Now
> you need to list all the tablespaces separately, to put them back into their
> own location.

I mean, don't you need to do that anyway, just in a more awkward way?
I don't understand who ever wants to keep track of their tablespaces
by either (a) source pathname or (b) OID rather than (c) user-visible
tablespace name.

> One thing I've been wondering about is an option to put the "new" tablespaces
> into a location relative to each of the old ones.
> --tablespace-relative-location=../restore-2024-04-18
> which would rewrite all the old tablespaces to that new location.

I think this would probably get limited use outside of testing scenarios.

> > I said (at least off-list) when that feature was introduced that there was
> > no way it was going to remain an isolated development hack, and I think
> > that's proved to be 100% correct.
>
> Hm, I guess I kinda agree. But not because I think it wasn't good for
> development, but because it'd often be much saner to use relative tablespaces
> than absolute ones even for prod.
>
> My only point here was that the test would be simpler if you
> a) didn't need to create a temp directory for the tablespace, both for
> primary and standby
> b) didn't need to "gin up" a tablespace map, because all the paths are
> relative
>
> Just to be clear: I don't want the above to block merging your test. If you
> think you want to do it the way you did, please do.

I think I will go ahead and do that soon, then. I'm totally fine with
it if somebody wants to do the testing in some other way, but this was
what made sense to me as the easiest way to adapt what's already
there. I think it's lame that init_from_backup() disclaims support for
tablespaces, as if tablespaces weren't a case that needs to be tested
heavily with respect to backups. And I think it's better to get
something merged that fixes the bug ASAP, and adds some test coverage,
and then if there's a better way to do that test coverage down the
road, well and good.

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