Re: Add FOREIGN to ALTER TABLE in pg_dump

Hello,
pg_dump creates plain ALTER TABLE statements even if the table is a foreign table, which for someone reading the dump is confusing.
This also made a difference when applying the dump if there is any plugin installed that hooks on ProcessUtility, because the plugin could react differently to ALTER TABLE than to ALTER FOREIGN TABLE. Opinions?

An unrelated question: if I apply pgindent to a file (in this case pg_dump.c) and get a bunch of changes on the indentation that are not related to my patch, which is the accepted policy? A different patch first with only the indentation? Maybe, am I using pgindent wrong?

Cheers
Luis M Carril

On 2019-Jul-12, Luis Carril wrote:

> Hello,
> pg_dump creates plain ALTER TABLE statements even if the table is a foreign table, which for someone reading the dump is confusing.
> This also made a difference when applying the dump if there is any plugin installed that hooks on ProcessUtility, because the plugin could react differently to ALTER TABLE than to ALTER FOREIGN TABLE. Opinions?

I think such a hook would be bogus, because it would miss anything done
by a user manually.

I don't disagree with adding FOREIGN, though.

Your patch is failing the pg_dump TAP tests. Please use
configure --enable-tap-tests, fix the problems, then resubmit.

> An unrelated question: if I apply pgindent to a file (in this case pg_dump.c) and get a bunch of changes on the indentation that are not related to my patch, which is the accepted policy? A different patch first with only the indentation? Maybe, am I using pgindent wrong?

We don't typically accept pgindent-only changes at random points in
the devel cycle.

I would suggest to run pgindent over the file and "git add -p" only the
changes that are relevant to your patch, discard the rest.
(Alternative: run pgindent, commit that, then apply your patch, pgindent
again and "git commit --amend", then "git rebase -i" and discard the
first pgindent commit. Your patch ends up pgindent-correct without
disturbing the rest of the file/tree).

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I don't disagree with adding FOREIGN, though.

Your patch is failing the pg_dump TAP tests. Please use
configure --enable-tap-tests, fix the problems, then resubmit.

Fixed, I've attached a new version.

Cheers
Luis M Carril

Hi,

On Thu, Sep 26, 2019 at 01:47:28PM +0000, Luis Carril wrote:
>
>I don't disagree with adding FOREIGN, though.
>
>Your patch is failing the pg_dump TAP tests. Please use
>configure --enable-tap-tests, fix the problems, then resubmit.
>
>Fixed, I've attached a new version.
>

This seems like a fairly small and non-controversial patch (I agree with
Alvaro that having the optional FOREIGN seems won't hurt). So barring
objections I'll polish it a bit and push sometime next week.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
>
>
> I don't disagree with adding FOREIGN, though.
>
> Your patch is failing the pg_dump TAP tests. Please use
> configure --enable-tap-tests, fix the problems, then resubmit.
>
> Fixed, I've attached a new version.

Will it be possible to add a test case for this, can we validate by
adding one test?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

vignesh C <vignesh21(at)gmail(dot)com> writes:
> On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
>>> Your patch is failing the pg_dump TAP tests. Please use
>>> configure --enable-tap-tests, fix the problems, then resubmit.

>> Fixed, I've attached a new version.

> Will it be possible to add a test case for this, can we validate by
> adding one test?

Isn't the change in the TAP test output sufficient?

regards, tom lane

On 2020-Jan-11, Tomas Vondra wrote:

> Hi,
>
> On Thu, Sep 26, 2019 at 01:47:28PM +0000, Luis Carril wrote:
> >
> > I don't disagree with adding FOREIGN, though.
> >
> > Your patch is failing the pg_dump TAP tests. Please use
> > configure --enable-tap-tests, fix the problems, then resubmit.
> >
> > Fixed, I've attached a new version.
>
> This seems like a fairly small and non-controversial patch (I agree with
> Alvaro that having the optional FOREIGN seems won't hurt). So barring
> objections I'll polish it a bit and push sometime next week.

If we're messing with that code, we may as well reduce cognitive load a
little bit and unify all those multiple consecutive appendStringInfo
calls into one. (My guess is that this was previously not possible
because there were multiple fmtId() calls in the argument list, but
that's no longer the case.)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> vignesh C <vignesh21(at)gmail(dot)com> writes:
> > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
> >>> Your patch is failing the pg_dump TAP tests. Please use
> >>> configure --enable-tap-tests, fix the problems, then resubmit.
>
> >> Fixed, I've attached a new version.
>
> > Will it be possible to add a test case for this, can we validate by
> > adding one test?
>
> Isn't the change in the TAP test output sufficient?
>

I could not see any expected file output changes in the patch. Should
we modify the existing test to validate this.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On 2020-Jan-14, vignesh C wrote:

> On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > vignesh C <vignesh21(at)gmail(dot)com> writes:
> > > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
> > >>> Your patch is failing the pg_dump TAP tests. Please use
> > >>> configure --enable-tap-tests, fix the problems, then resubmit.
> >
> > >> Fixed, I've attached a new version.
> >
> > > Will it be possible to add a test case for this, can we validate by
> > > adding one test?
> >
> > Isn't the change in the TAP test output sufficient?
>
> I could not see any expected file output changes in the patch. Should
> we modify the existing test to validate this.

Yeah, I think there should be at least one regexp in t/002_pg_dump.pl to
verify ALTER FOREIGN TABLE is being produced.

I wonder if Tom is thinking about Luis' other pg_dump patch for foreign
tables, which includes some changes to src/test/modules/test_pg_dump.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Isn't the change in the TAP test output sufficient?

> Yeah, I think there should be at least one regexp in t/002_pg_dump.pl to
> verify ALTER FOREIGN TABLE is being produced.
> I wonder if Tom is thinking about Luis' other pg_dump patch for foreign
> tables, which includes some changes to src/test/modules/test_pg_dump.

No, I was just reacting to the comment that the TAP test was failing,
and assuming that that meant the patch had already changed the expected
output. Looking at the patch now, I suppose that just means it had
incautiously changed whitespace or something for the non-foreign case.

I can't get terribly excited about persuading that test to cover this
trivial little bit of logic, but if you are, I won't stand in the way.

regards, tom lane

On 2020-Jan-14, Tom Lane wrote:

> I can't get terribly excited about persuading that test to cover this
> trivial little bit of logic, but if you are, I won't stand in the way.

Hmm, that's a good point actually: the patch changed several places to
inject the FOREIGN keyword, so in order to cover them all it would need
several additional regexps, not just one. I'm not sure that
002_pg_dump.pl is prepared to do that without unsightly contortions.

Anyway, other than that minor omission the patch seemed good to me, so I
don't oppose Tomas pushing the version I posted yesterday. Or I can, if
he prefers that.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 15 Jan 2020, at 00:04, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2020-Jan-14, Tom Lane wrote:
>
>> I can't get terribly excited about persuading that test to cover this
>> trivial little bit of logic, but if you are, I won't stand in the way.
>
> Hmm, that's a good point actually: the patch changed several places to
> inject the FOREIGN keyword, so in order to cover them all it would need
> several additional regexps, not just one. I'm not sure that
> 002_pg_dump.pl is prepared to do that without unsightly contortions.

I agree that it doesn't seem worth holding up this patch for that, even though
it would be nice if we do add a test at some point.

> Anyway, other than that minor omission the patch seemed good to me, so I
> don't oppose Tomas pushing the version I posted yesterday. Or I can, if
> he prefers that.

This patch still applies with some offsets and a bit of fuzz, and looking over
the patch I agree with Alvaro.

Moving this patch to Ready for Committer.

cheers ./daniel

On 2020-Mar-19, Daniel Gustafsson wrote:

> Moving this patch to Ready for Committer.

Thanks, pushed.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services