Re: Improving test coverage of extensions with pg_dump

On Wed, Sep 23, 2015 at 1:04 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Euler Taveira wrote:
> > On 17-09-2015 14:21, Michael Paquier wrote:
> > >pg_dump relies on attnum to define the column ordering, so one
> > >possibility would be to do things more consistently at backend level.
>
> We discussed this in some other thread, not long ago. I looked briefly
> in the archives but couldn't find it. I think the conclusion was
> something along the lines of "hmm, tough".
>

That's hours, even days of fun ahead for sure.

> > Someone can say that we could assign an attnum for column "d" considering
> > all of the inheritance tree. However, attnum is used as an index to
> arrays
> > (we could bloat some of those) and some logic rely on it to count the
> number
> > of columns. It would become tablecmds.c into an spaghetti.
>

That was my first impression, but that's just a band-aid. Switching the
logic in stable branches to assign a correct attnum for a child table,
while switching on the way the attnum referring to the parent entries is a
scary idea. I would suspect that this would break many other things for
fixing a narrow case, and it is still possible for the user to get away by
using COPY or INSERT with the list of columns listed when taking a dump.

We don't need any more spaghetti there, thanks!
>

Agreed.

> > IMHO a possible way to solve it is adding support for logical column
> > ordering. An ALTER TABLE command (emitted if a parameter was informed)
> > during dump could handle it. BTW, last thread [1] about logical column
> > ordering seems to have died a few months ago. Alvaro?
>
> Tomas Vondra also worked a bit on this patch, and we eventually gave up
> on it due to lack of time. We might be able to get back on it someday,
> but do not hold your breath. If you want the current bug fixed, do not
> wait for logical column numbering.
>

Honestly I have the feeling that the discussion of this thread gets
unproductive, let's not forget that the patch presented on this thread is
just aiming at adding one test case to ensure that extensions using
dumpable relations with FKs get correctly dumped, which is to ensure that
we do not reintroduce a bug that existed in the extension facility since
its introduction in 9.1. That being said, the patch is just fine for this
purpose, but that's just my opinion.
--
Michael