Re: pg_dump is broken for partition tablespaces

On Mon, Apr 22, 2019 at 4:43 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Well, frequently when people discuss ideas on this list, others discuss
> and provide further ideas to try help to find a working solution, rather
> than throw every roadblock they can think of (though roadblocks are
> indeed thrown now and then).  If I've taken a long time to find a
> working solution, maybe it's because I have no shoulders of giants to
> stand on, and I'm a pretty short guy, so I need to build me a ladder.

What exactly do you mean by throwing up roadblocks?  I don't have a
basket full of great ideas for how to solve this problem that I'm
failing to suggest out of some sort of perverse desire to see you
fail.  I'm not quite as convinced as Tom and Andres that this whole
idea is fatally flawed and can't ever be made to work correctly, but I
think it's quite possible that they are right, both because their
objections sound to me like they are target and because they are
pretty smart people.  But that's also because I haven't spent a lot of
time on this issue, which I think is pretty fair, because it seems
like it would be unfair to complain that I am not spending enough time
helping fix code that I advised against committing in the first place.
How much time should I spent giving you advice if my previous advice
was ignored?

But FWIW, it seems to me that a good place to start solving this
problem would be to think hard about what Andres said here:

http://postgr.es/m/20190306161744.22jdkg37fyi2zyke@alap3.anarazel.de

Specifically, this complaint: "I still think the feature as is doesn't
seem to have very well defined behaviour."

If we know what the feature is supposed to do, then it should be
possible to look at each relevant piece of code and decides whether it
implements the specification correctly or not.  But if we don't have a
specification -- that is, we don't know precisely what the feature is
supposed to do -- then we'll just end up whacking the behavior around
trying to get some combination that makes sense, and the whole effort
is probably doomed.

I think that the large quote block from David Rowley in the middle of
the above-linked email gets at the definitional problem pretty
clearly: the documentation seems to be intending to say -- although it
is not 100% clear -- that if TABLESPACE is specified it has effect on
all future children, and if not specified then those children get the
tablespace they would have gotten anyway.  But that behavior is
impossible to implement correctly unless there is a way to distinguish
between a partitioned table for which TABLESPACE was not specified and
where it was specified to be the same as the default tablespace for
the database.  And we know, per previous conversation, that the
catalog representation admits of no way to make that distinction.
Using reltablespace = dattablespace is clearly the wrong answer,
because that breaks stuff.  Tom earlier suggested, I believe, that
some fixed OID could be used, e.g. reltablespace = 1 means "tablespace
not specified" and reltablespace = 0 means dattablespace.  That should
be safe enough, since I don't think OID 1 can ever be the OID of a
real tablespace.  I think this is probably the only way forward if
this definition captures the desired behavior.

The other option is to decide that we want some other behavior.  In
that case, the way forward depends on what we want the behavior to be.
Your proposal upthread is to disallow the case where the user provides
an explicit TABLESPACE setting whose value matches the default
tablespace for the database.  But that definition seems to have an
obvious problem: just because that's not true when the partitioned
table is defined doesn't mean it won't become true later, because the
default tablespace for the database can be changed, or the database
can be copied and the copy be assigned a different default tablespace.
Even if there were no issue, that definition doesn't sound very clean,
because it means that reltablespace = 0 for a regular relation means
dattablespace and for a partitioned relation it means none.

Now that's not the only way we could go either, I suppose.  There must
be a variety of other possible behaviors.  But the one Tom and Andres
are proposing -- go back to the way it worked in v10 -- is definitely
not crazy.  You are right that a lot of people didn't like that, but
the definition was absolutely clear, because it was the exact same
definition we use for normal tables, with the straigthforward
extension that for relations with no storage it meant nothing.

Going back to the proposal of making OID = 0 mean TABLESPACE
dattablespace, OID = 1 meaning no TABLESPACE clause specified for this
partitioned table, and OID = whatever meaning that particular
non-default tablespace, I do see a couple of problems with that idea
too:

- I don't have any idea how to salvage things in v11, where the
catalog representation is irredeemably ambiguous.  We'd probably have
to be satisfied with fairly goofy behavior in v11.

- It's not enough to have a good catalog representation.  You also
have to be able to recreate those catalog states.  I think you
probably need a way to set the reltablespace to whatever value you
need it to have without changing the tables under it.  Like ALTER
TABLE ONLY blah {TABLESPACE some_tablespace_name | NO TABLESPACE} or
some such thing.  That exact idea may be wrong, but I think we are not
going to have much luck getting to a happy place without something of
this sort.

If we have a clear definition of what the feature does, a catalog
representation to match, and syntax that can recreate any given
catalog representation, then this should be an SMOP.

But again, rip it all out and try it again someday is not a crazy
proposal, IMHO.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company