Thread: 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
Luis M Carril
Attachment
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 dumpis confusing. > This also made a difference when applying the dump if there is any plugin installed that hooks on ProcessUtility, becausethe 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 indentationthat 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
Fixed, I've attached a new version.
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.
Cheers
Luis M Carril
Attachment
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.carril@swarm64.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@gmail.com> writes:
> On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis.carril@swarm64.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
Attachment
On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > vignesh C <vignesh21@gmail.com> writes: > > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis.carril@swarm64.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@sss.pgh.pa.us> wrote: > > > > vignesh C <vignesh21@gmail.com> writes: > > > On Thu, Sep 26, 2019 at 7:17 PM Luis Carril <luis.carril@swarm64.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@2ndquadrant.com> writes:
>> On Mon, Jan 13, 2020 at 7:52 PM Tom Lane <tgl@sss.pgh.pa.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@2ndquadrant.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