Add FOREIGN to ALTER TABLE in pg_dump

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Add FOREIGN to ALTER TABLE in pg_dump

Luis Carril
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

0001-Add-FOREIGN-to-ALTER-statements.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Luis Carril


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



0001-Add-FOREIGN-to-ALTER-statements-v2.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Tomas Vondra-4
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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

vignesh C
In reply to this post by Luis Carril
On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Tom Lane-2
vignesh C <[hidden email]> writes:
> On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Alvaro Herrera-9
In reply to this post by Tomas Vondra-4
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

v3-0001-pg_dump-Add-FOREIGN-to-ALTER-statements-if-approp.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

vignesh C
In reply to this post by Tom Lane-2
On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <[hidden email]> wrote:

>
> vignesh C <[hidden email]> writes:
> > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Alvaro Herrera-9
On 2020-Jan-14, vignesh C wrote:

> On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <[hidden email]> wrote:
> >
> > vignesh C <[hidden email]> writes:
> > > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
>> On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add FOREIGN to ALTER TABLE in pg_dump

Alvaro Herrera-9
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