agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedpg_dump restore time and Foreign Keys
40+ messages / 12 participants
[nested] [flat]
* pg_dump restore time and Foreign Keys
@ 2008-06-05 06:23 Simon Riggs <simon@2ndquadrant.com>
0 siblings, 3 replies; 40+ messages in thread
From: Simon Riggs @ 2008-06-05 06:23 UTC (permalink / raw)
To: pgsql-hackers
pg_dump restore times can be high when they include many ALTER TABLE ADD
FORIEGN KEY statements, since each statement checks the data to see if
it is fully valid in all cases.
I've been asked "why we run that at all?", since if we dumped the tables
together, we already know they match.
If we had a way of pg_dump passing on the information that the test
already passes, we would be able to skip the checks.
Proposal:
* Introduce a new mode for ALTER TABLE ADD FOREIGN KEY [WITHOUT CHECK];
When we run WITHOUT CHECK, iff both the source and target table are
newly created in this transaction, then we skip the check. If the check
is skipped we mark the constraint as being unchecked, so we can tell
later if this has been used.
* Have pg_dump write the new syntax into its dumps, when both the source
and target table are dumped in same run
I'm guessing that the WITHOUT CHECK option would not be acceptable as an
unprotected trap for our lazy and wicked users. :-)
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 07:19 Heikki Linnakangas <heikki@enterprisedb.com>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 1 reply; 40+ messages in thread
From: Heikki Linnakangas @ 2008-06-05 07:19 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
> I'm guessing that the WITHOUT CHECK option would not be acceptable as an
> unprotected trap for our lazy and wicked users. :-)
Yes, that sounds scary.
Instead, I'd suggest finding ways to speed up the ALTER TABLE ADD
FOREIGN KEY. Or speeding up COPY into a table with foreign keys already
defined. For example, you might want to build an in-memory hash table of
the keys in the target table, instead of issuing a query on each INSERT,
if the target table isn't huge.
Nothing beats the speed of simply not checking the constraint, of
course, but I'd hate to lose the protection it gives.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 07:45 Simon Riggs <simon@2ndquadrant.com>
parent: Heikki Linnakangas <heikki@enterprisedb.com>
0 siblings, 2 replies; 40+ messages in thread
From: Simon Riggs @ 2008-06-05 07:45 UTC (permalink / raw)
To: Heikki Linnakangas <heikki@enterprisedb.com>; +Cc: pgsql-hackers
On Thu, 2008-06-05 at 10:19 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > I'm guessing that the WITHOUT CHECK option would not be acceptable as an
> > unprotected trap for our lazy and wicked users. :-)
>
> Yes, that sounds scary.
>
> Instead, I'd suggest finding ways to speed up the ALTER TABLE ADD
> FOREIGN KEY.
I managed a suggestion for improving it for integers only, but if
anybody has any other ideas, I'm all ears.
> Or speeding up COPY into a table with foreign keys already
> defined. For example, you might want to build an in-memory hash table of
> the keys in the target table, instead of issuing a query on each INSERT,
> if the target table isn't huge.
No, that's not the problem, but I agree that is a problem also.
> Nothing beats the speed of simply not checking the constraint, of
> course, but I'd hate to lose the protection it gives.
Are you saying you don't like the rest of the proposal, or just don't
like the idea of having that added as an unprotected option, but find
the proposal acceptable?
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 08:54 Richard Huxton <dev@archonet.com>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 0 replies; 40+ messages in thread
From: Richard Huxton @ 2008-06-05 08:54 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
>
> If we had a way of pg_dump passing on the information that the test
> already passes, we would be able to skip the checks.
>
> Proposal:
>
> * Introduce a new mode for ALTER TABLE ADD FOREIGN KEY [WITHOUT CHECK];
> * Have pg_dump write the new syntax into its dumps, when both the source
> and target table are dumped in same I've been known to manually tweak dumps before now. I can see me
forgetting this.
What about pg_dump writing out a row-count and MD5 of the rows in the
COPY (just a textual calculation). Iff the restore checksum matches the
dump checksum for both tables then the foreign-keys can be skipped.
If the restore checksum doesn't match the dump then it can issue a
warning, but continue and run the full fkey check.
--
Richard Huxton
Archonet Ltd
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 11:57 Andrew Dunstan <andrew@dunslane.net>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 1 reply; 40+ messages in thread
From: Andrew Dunstan @ 2008-06-05 11:57 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
> pg_dump restore times can be high when they include many ALTER TABLE ADD
> FORIEGN KEY statements, since each statement checks the data to see if
> it is fully valid in all cases.
>
> I've been asked "why we run that at all?", since if we dumped the tables
> together, we already know they match.
>
> If we had a way of pg_dump passing on the information that the test
> already passes, we would be able to skip the checks.
>
> Proposal:
>
> * Introduce a new mode for ALTER TABLE ADD FOREIGN KEY [WITHOUT CHECK];
> When we run WITHOUT CHECK, iff both the source and target table are
> newly created in this transaction, then we skip the check. If the check
> is skipped we mark the constraint as being unchecked, so we can tell
> later if this has been used.
>
> * Have pg_dump write the new syntax into its dumps, when both the source
> and target table are dumped in same run
>
> I'm guessing that the WITHOUT CHECK option would not be acceptable as an
> unprotected trap for our lazy and wicked users. :-)
>
This whole proposal would be a major footgun which would definitely be
abused, IMNSHO.
I think Heikki's idea of speeding up the check using a hash table of the
foreign keys possibly has merit.
cheers
andrew
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 12:23 Heikki Linnakangas <heikki@enterprisedb.com>
parent: Simon Riggs <simon@2ndquadrant.com>
1 sibling, 0 replies; 40+ messages in thread
From: Heikki Linnakangas @ 2008-06-05 12:23 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
> Are you saying you don't like the rest of the proposal, or just don't
> like the idea of having that added as an unprotected option, but find
> the proposal acceptable?
I don't like the idea of having an unprotected option. If we were going
to have one, I wouldn't bother with the extra checks you proposed; it's
going to be unsafe anyway.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 12:56 Simon Riggs <simon@2ndquadrant.com>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-05 12:56 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: pgsql-hackers
On Thu, 2008-06-05 at 07:57 -0400, Andrew Dunstan wrote:
>
> Simon Riggs wrote:
> > pg_dump restore times can be high when they include many ALTER TABLE ADD
> > FORIEGN KEY statements, since each statement checks the data to see if
> > it is fully valid in all cases.
> >
> > I've been asked "why we run that at all?", since if we dumped the tables
> > together, we already know they match.
> >
> > If we had a way of pg_dump passing on the information that the test
> > already passes, we would be able to skip the checks.
> >
> > Proposal:
> >
> > * Introduce a new mode for ALTER TABLE ADD FOREIGN KEY [WITHOUT CHECK];
> > When we run WITHOUT CHECK, iff both the source and target table are
> > newly created in this transaction, then we skip the check. If the check
> > is skipped we mark the constraint as being unchecked, so we can tell
> > later if this has been used.
> >
> > * Have pg_dump write the new syntax into its dumps, when both the source
> > and target table are dumped in same run
> >
> > I'm guessing that the WITHOUT CHECK option would not be acceptable as an
> > unprotected trap for our lazy and wicked users. :-)
> >
>
> This whole proposal would be a major footgun which would definitely be
> abused, IMNSHO.
OK, understood. Two negatives is enough to sink it.
> I think Heikki's idea of speeding up the check using a hash table of the
> foreign keys possibly has merit.
The query is sent through SPI, so if there was a way to speed this up,
we would already be using it implicitly. If we find a way to speed up
joins it will improve the FK check also.
The typical join plan for the check query is already a hash join,
assuming the target table is small enough. If not, its a huge sort/merge
join. So in a way, we already follow the suggestion.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 13:01 Heikki Linnakangas <heikki@enterprisedb.com>
parent: Simon Riggs <simon@2ndquadrant.com>
1 sibling, 1 reply; 40+ messages in thread
From: Heikki Linnakangas @ 2008-06-05 13:01 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
> On Thu, 2008-06-05 at 10:19 +0300, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>>> I'm guessing that the WITHOUT CHECK option would not be acceptable as an
>>> unprotected trap for our lazy and wicked users. :-)
>> Yes, that sounds scary.
>>
>> Instead, I'd suggest finding ways to speed up the ALTER TABLE ADD
>> FOREIGN KEY.
>
> I managed a suggestion for improving it for integers only, but if
> anybody has any other ideas, I'm all ears.
Well, one idea would be to allow adding multiple foreign keys in one
command, and checking them all at once with one SQL query instead of one
per foreign key. Right now we need one seq scan over the table per
foreign key, by checking all references at once we would only need one
seq scan to check them all.
>> Or speeding up COPY into a table with foreign keys already
>> defined. For example, you might want to build an in-memory hash table of
>> the keys in the target table, instead of issuing a query on each INSERT,
>> if the target table isn't huge.
>
> No, that's not the problem, but I agree that is a problem also.
It is related, because if we can make COPY into a table with foreign
keys fast enough, we could rearrange dumps so that foreign keys are
created before loading data. That would save the seqscan over the table
altogether.
Thinking about this idea a bit more, instead of loading the whole target
table into memory, it would probably make more sense to keep a hash
table as just a cache of the most recent keys that have been referenced.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 13:27 Simon Riggs <simon@2ndquadrant.com>
parent: Heikki Linnakangas <heikki@enterprisedb.com>
0 siblings, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-05 13:27 UTC (permalink / raw)
To: Heikki Linnakangas <heikki@enterprisedb.com>; +Cc: pgsql-hackers
On Thu, 2008-06-05 at 16:01 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Thu, 2008-06-05 at 10:19 +0300, Heikki Linnakangas wrote:
> >> Simon Riggs wrote:
> >>> I'm guessing that the WITHOUT CHECK option would not be acceptable as an
> >>> unprotected trap for our lazy and wicked users. :-)
> >> Yes, that sounds scary.
> >>
> >> Instead, I'd suggest finding ways to speed up the ALTER TABLE ADD
> >> FOREIGN KEY.
> >
> > I managed a suggestion for improving it for integers only, but if
> > anybody has any other ideas, I'm all ears.
>
> Well, one idea would be to allow adding multiple foreign keys in one
> command, and checking them all at once with one SQL query instead of one
> per foreign key. Right now we need one seq scan over the table per
> foreign key, by checking all references at once we would only need one
> seq scan to check them all.
No need. Just parallelise the restore with concurrent psql. Which would
speed up the index creation also. Does Greg have plans for further work?
> >> Or speeding up COPY into a table with foreign keys already
> >> defined. For example, you might want to build an in-memory hash table of
> >> the keys in the target table, instead of issuing a query on each INSERT,
> >> if the target table isn't huge.
> >
> > No, that's not the problem, but I agree that is a problem also.
>
> It is related, because if we can make COPY into a table with foreign
> keys fast enough, we could rearrange dumps so that foreign keys are
> created before loading data. That would save the seqscan over the table
> altogether.
True.
> Thinking about this idea a bit more, instead of loading the whole target
> table into memory, it would probably make more sense to keep a hash
> table as just a cache of the most recent keys that have been referenced.
If you can think of a way of improving hash joins generally, then it
will work for this specific case also.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-05 13:35 Heikki Linnakangas <heikki@enterprisedb.com>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 0 replies; 40+ messages in thread
From: Heikki Linnakangas @ 2008-06-05 13:35 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: pgsql-hackers; Greg Stark <greg.stark@enterprisedb.com>
Simon Riggs wrote:
> On Thu, 2008-06-05 at 16:01 +0300, Heikki Linnakangas wrote:
>> Well, one idea would be to allow adding multiple foreign keys in one
>> command, and checking them all at once with one SQL query instead of one
>> per foreign key. Right now we need one seq scan over the table per
>> foreign key, by checking all references at once we would only need one
>> seq scan to check them all.
>
> No need. Just parallelise the restore with concurrent psql. Which would
> speed up the index creation also.
True, you could do that.
> Does Greg have plans for further work?
I believe he's busy with other stuff at the moment.
>> Thinking about this idea a bit more, instead of loading the whole target
>> table into memory, it would probably make more sense to keep a hash
>> table as just a cache of the most recent keys that have been referenced.
>
> If you can think of a way of improving hash joins generally, then it
> will work for this specific case also.
Individual RI checks performed on inserts/COPY don't do a hash join. The
bulk check done by ALTER TABLE ADD FOREIGN KEY does, but that's
different issue.
This hash table would be a specific trick to speed up RI checks. If
you're anyway I/O bound, it wouldn't help, and you'd already be better
off creating the foreign key first and loading the data after that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-07 17:08 Robert Treat <xzilla@users.sourceforge.net>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 1 reply; 40+ messages in thread
From: Robert Treat @ 2008-06-07 17:08 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Simon Riggs <simon@2ndquadrant.com>; Andrew Dunstan <andrew@dunslane.net>
On Thursday 05 June 2008 08:56:35 Simon Riggs wrote:
> On Thu, 2008-06-05 at 07:57 -0400, Andrew Dunstan wrote:
> > Simon Riggs wrote:
> > > pg_dump restore times can be high when they include many ALTER TABLE
> > > ADD FORIEGN KEY statements, since each statement checks the data to see
> > > if it is fully valid in all cases.
> > >
> > > I've been asked "why we run that at all?", since if we dumped the
> > > tables together, we already know they match.
> > >
> > > If we had a way of pg_dump passing on the information that the test
> > > already passes, we would be able to skip the checks.
> > >
> > > Proposal:
> > >
> > > * Introduce a new mode for ALTER TABLE ADD FOREIGN KEY [WITHOUT CHECK];
> > > When we run WITHOUT CHECK, iff both the source and target table are
> > > newly created in this transaction, then we skip the check. If the check
> > > is skipped we mark the constraint as being unchecked, so we can tell
> > > later if this has been used.
> > >
> > > * Have pg_dump write the new syntax into its dumps, when both the
> > > source and target table are dumped in same run
> > >
> > > I'm guessing that the WITHOUT CHECK option would not be acceptable as
> > > an unprotected trap for our lazy and wicked users. :-)
> >
> > This whole proposal would be a major footgun which would definitely be
> > abused, IMNSHO.
>
> OK, understood. Two negatives is enough to sink it.
>
Heh, I would have argued that the idea should go the other way and just make
this part of the normal syntax. Oracle DBA's have been doing this for years
(MS SQL supports it too actually) and it really helps working around having
to hold locks on large relations for lengthy periods of times. Heck, I'd like
to see a no check option for all constraints really.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-07 17:41 Simon Riggs <simon@2ndquadrant.com>
parent: Robert Treat <xzilla@users.sourceforge.net>
0 siblings, 2 replies; 40+ messages in thread
From: Simon Riggs @ 2008-06-07 17:41 UTC (permalink / raw)
To: Robert Treat <xzilla@users.sourceforge.net>; +Cc: pgsql-hackers; Andrew Dunstan <andrew@dunslane.net>
On Sat, 2008-06-07 at 13:08 -0400, Robert Treat wrote:
> On Thursday 05 June 2008 08:56:35 Simon Riggs wrote:
> > On Thu, 2008-06-05 at 07:57 -0400, Andrew Dunstan wrote:
> Heh, I would have argued that the idea should go the other way and
> just make this part of the normal syntax. Oracle DBA's have been
> doing this for years (MS SQL supports it too actually) and it really
> helps working around having to hold locks on large relations for
> lengthy periods of times. Heck, I'd like to see a no check option for
> all constraints really.
Interesting that SQL Server does it also.
Holding the lock for a long period is just one more problem. :-)
I'm always torn between the I-know-what-Im-doing-so-give-me-the-option
viewpoint and the some-dumbass-will-abuse-it viewpoint. I see the
results of both viewpoints daily.
Perhaps we need a GUC that says expert_mode = on. In expert_mode we are
allowed to do a range of things that are normally avoided - there would
be an explicit list. Managers can then take a single considered decision
as to whether the situation warrants extreme action and their DBA is
good enough to handle it. That might resolve our continued angst about
whether our users our smart enough to avoid the gotchas, or just smart
enough to win a DBA's Darwin Award.
The UNIX philosophy has always been to allow the power to exist, yet
seek to minimise the number of people who exercise it. Another idea
might be to make such command options superuser only, to ensure the
power is available, yet only in the hands of, by-definition, the trusted
few.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-07 19:00 Andrew Dunstan <andrew@dunslane.net>
parent: Simon Riggs <simon@2ndquadrant.com>
1 sibling, 1 reply; 40+ messages in thread
From: Andrew Dunstan @ 2008-06-07 19:00 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Simon Riggs wrote:
> On Sat, 2008-06-07 at 13:08 -0400, Robert Treat wrote:
>
>> On Thursday 05 June 2008 08:56:35 Simon Riggs wrote:
>>
>>> On Thu, 2008-06-05 at 07:57 -0400, Andrew Dunstan wrote:
>>>
>
>
>> Heh, I would have argued that the idea should go the other way and
>> just make this part of the normal syntax. Oracle DBA's have been
>> doing this for years (MS SQL supports it too actually) and it really
>> helps working around having to hold locks on large relations for
>> lengthy periods of times. Heck, I'd like to see a no check option for
>> all constraints really.
>>
>
> Interesting that SQL Server does it also.
>
> Holding the lock for a long period is just one more problem. :-)
>
> I'm always torn between the I-know-what-Im-doing-so-give-me-the-option
> viewpoint and the some-dumbass-will-abuse-it viewpoint. I see the
> results of both viewpoints daily.
>
> Perhaps we need a GUC that says expert_mode = on. In expert_mode we are
> allowed to do a range of things that are normally avoided - there would
> be an explicit list. Managers can then take a single considered decision
> as to whether the situation warrants extreme action and their DBA is
> good enough to handle it. That might resolve our continued angst about
> whether our users our smart enough to avoid the gotchas, or just smart
> enough to win a DBA's Darwin Award.
>
> The UNIX philosophy has always been to allow the power to exist, yet
> seek to minimise the number of people who exercise it. Another idea
> might be to make such command options superuser only, to ensure the
> power is available, yet only in the hands of, by-definition, the trusted
> few.
>
>
If we go down this road then I would far rather we tried to devise some
safe (or semi-safe) way of doing it instead of simply providing expert
(a.k.a. footgun) mode.
For instance, I'm wondering if we could do something with checksums of
the input lines or something else that would make this difficult to do
in circumstances other than pg_restore.
cheers
andrew
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-07 20:22 Tom Lane <tgl@sss.pgh.pa.us>
parent: Simon Riggs <simon@2ndquadrant.com>
1 sibling, 1 reply; 40+ messages in thread
From: Tom Lane @ 2008-06-07 20:22 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers; Andrew Dunstan <andrew@dunslane.net>
Simon Riggs <simon@2ndquadrant.com> writes:
> Perhaps we need a GUC that says expert_mode = on. ... Another idea
> might be to make such command options superuser only, to ensure the
> power is available, yet only in the hands of, by-definition, the trusted
> few.
This all seems pretty useless, as the sort of user most likely to shoot
himself in the foot will also always be running as superuser.
I'd much rather see us expend more effort on speeding up the checks
than open holes in the system.
regards, tom lane
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-08 23:45 Robert Treat <xzilla@users.sourceforge.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 40+ messages in thread
From: Robert Treat @ 2008-06-08 23:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Simon Riggs <simon@2ndquadrant.com>; pgsql-hackers; Andrew Dunstan <andrew@dunslane.net>
On Saturday 07 June 2008 16:22:56 Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > Perhaps we need a GUC that says expert_mode = on. ... Another idea
> > might be to make such command options superuser only, to ensure the
> > power is available, yet only in the hands of, by-definition, the trusted
> > few.
>
> This all seems pretty useless, as the sort of user most likely to shoot
> himself in the foot will also always be running as superuser.
>
yeah, i'm not a big fan of "set enable_footgun=true" since the people likely
to get tripped up are going to blindly enable these modes.
otoh, if we do such a thing, i would be a big fan of calling
it "enable_footgun" :-)
> I'd much rather see us expend more effort on speeding up the checks
> than open holes in the system.
>
and i'm sure no one is against that idea, but you're never going to be able to
match the performance of just avoiding the check.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 00:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Treat <xzilla@users.sourceforge.net>
0 siblings, 1 reply; 40+ messages in thread
From: Tom Lane @ 2008-06-09 00:12 UTC (permalink / raw)
To: Robert Treat <xzilla@users.sourceforge.net>; +Cc: Simon Riggs <simon@2ndquadrant.com>; pgsql-hackers; Andrew Dunstan <andrew@dunslane.net>
Robert Treat <xzilla@users.sourceforge.net> writes:
> and i'm sure no one is against that idea, but you're never going to be able to
> match the performance of just avoiding the check.
We'll never be able to match the performance of not having transactions,
either, but the community has never for a moment considered having a
"no transactions" mode.
regards, tom lane
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 00:40 Robert Treat <xzilla@users.sourceforge.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 40+ messages in thread
From: Robert Treat @ 2008-06-09 00:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Simon Riggs <simon@2ndquadrant.com>; pgsql-hackers; Andrew Dunstan <andrew@dunslane.net>
On Sunday 08 June 2008 20:12:15 Tom Lane wrote:
> Robert Treat <xzilla@users.sourceforge.net> writes:
> > and i'm sure no one is against that idea, but you're never going to be
> > able to match the performance of just avoiding the check.
>
> We'll never be able to match the performance of not having transactions,
> either, but the community has never for a moment considered having a
> "no transactions" mode.
>
it's unclear what a "no transaction" mode would mean, but I'd be willing to
guess some people have consider aspects of it (we've just never had
agreement)
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 12:43 Decibel! <decibel@decibel.org>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 2 replies; 40+ messages in thread
From: Decibel! @ 2008-06-09 12:43 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Simon Riggs <simon@2ndquadrant.com>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
On Jun 7, 2008, at 2:00 PM, Andrew Dunstan wrote:
> If we go down this road then I would far rather we tried to devise
> some safe (or semi-safe) way of doing it instead of simply
> providing expert (a.k.a. footgun) mode.
>
> For instance, I'm wondering if we could do something with checksums
> of the input lines or something else that would make this difficult
> to do in circumstances other than pg_restore.
Yes, but that provides no help at all outside of pg_dump. Being able
to add a FK with NO CHECK would be tremendously useful outside of
pg_dump. Actually, in the interest of stating the problem and not the
solution, what we need is a way to add FKs that doesn't lock
everything up to perform the key checks. Perhaps there is some semi-
safe way that the constraint could be added and the checks done in
the background...
As for the footgun aspect, are we the enterprise-class OSS database
or the one that caters itself to noobs that will go out of their way
to make life hard on themselves? I'm all in favor of not adding
footguns that don't have value, but this one holds a lot of value for
anyone trying to maintain a large database in a 24/7 environment. To
put this in perspective, the amount of revenue we would loose from
adding just one FK to one of our larger tables would more than cover
paying someone to develop this feature.
--
Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828
Attachments:
[application/pkcs7-signature] smime.p7s (2.5K, ../../E2AE91BB-0FCE-44CE-AC3B-200D6F1DD0BC@decibel.org/2-smime.p7s)
download
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 14:00 Andrew Dunstan <andrew@dunslane.net>
parent: Decibel! <decibel@decibel.org>
1 sibling, 0 replies; 40+ messages in thread
From: Andrew Dunstan @ 2008-06-09 14:00 UTC (permalink / raw)
To: Decibel! <decibel@decibel.org>; +Cc: Simon Riggs <simon@2ndquadrant.com>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Decibel! wrote:
>
>
> Yes, but that provides no help at all outside of pg_dump. Being able
> to add a FK with NO CHECK would be tremendously useful outside of
> pg_dump. Actually, in the interest of stating the problem and not the
> solution, what we need is a way to add FKs that doesn't lock
> everything up to perform the key checks. Perhaps there is some
> semi-safe way that the constraint could be added and the checks done
> in the background...
I had some thoughts along the same lines.
But how do you propose to recover when the check fails? What should
pg_restore do if the dump is corrupt causing an FK check to fail?
I suppose we could have some sort of marking for FK constraints along
the lines of {checked, unchecked, invalid}.
>
> As for the footgun aspect, are we the enterprise-class OSS database or
> the one that caters itself to noobs that will go out of their way to
> make life hard on themselves?
We are the database that tries very hard to keep its promises. If you
want to change or relax those promises then the implications need to be
very very clear.
> I'm all in favor of not adding footguns that don't have value, but
> this one holds a lot of value for anyone trying to maintain a large
> database in a 24/7 environment. To put this in perspective, the amount
> of revenue we would loose from adding just one FK to one of our larger
> tables would more than cover paying someone to develop this feature.
>
Come up with a good proposal and I'm your man :-) I haven't seen one yet.
cheers
andrew
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 14:57 Tom Lane <tgl@sss.pgh.pa.us>
parent: Decibel! <decibel@decibel.org>
1 sibling, 1 reply; 40+ messages in thread
From: Tom Lane @ 2008-06-09 14:57 UTC (permalink / raw)
To: Decibel! <decibel@decibel.org>; +Cc: Andrew Dunstan <andrew@dunslane.net>; Simon Riggs <simon@2ndquadrant.com>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Decibel! <decibel@decibel.org> writes:
> Actually, in the interest of stating the problem and not the
> solution, what we need is a way to add FKs that doesn't lock
> everything up to perform the key checks.
Ah, finally a useful comment. I think it might be possible to do an
"add FK concurrently" type of command that would take exclusive lock
for just long enough to add the triggers, then scan the tables with just
AccessShareLock to see if the existing rows meet the constraint, and
if so finally mark the constraint "valid". Meanwhile the constraint
would be enforced against newly-added rows by the triggers, so nothing
gets missed. You'd still get a small hiccup in system performance
from the transient exclusive lock, but nothing like as bad as it is
now. Would that solve your problem?
regards, tom lane
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:12 Simon Riggs <simon@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 3 replies; 40+ messages in thread
From: Simon Riggs @ 2008-06-09 15:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
> Decibel! <decibel@decibel.org> writes:
> > Actually, in the interest of stating the problem and not the
> > solution, what we need is a way to add FKs that doesn't lock
> > everything up to perform the key checks.
>
> Ah, finally a useful comment. I think it might be possible to do an
> "add FK concurrently" type of command that would take exclusive lock
> for just long enough to add the triggers, then scan the tables with just
> AccessShareLock to see if the existing rows meet the constraint, and
> if so finally mark the constraint "valid". Meanwhile the constraint
> would be enforced against newly-added rows by the triggers, so nothing
> gets missed. You'd still get a small hiccup in system performance
> from the transient exclusive lock, but nothing like as bad as it is
> now. Would that solve your problem?
That's good, but it doesn't solve the original user complaint about
needing to re-run many, many large queries to which we already know the
answer.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:23 Andrew Dunstan <andrew@dunslane.net>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 1 reply; 40+ messages in thread
From: Andrew Dunstan @ 2008-06-09 15:23 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Simon Riggs wrote:
> On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
>
>> Decibel! <decibel@decibel.org> writes:
>>
>>> Actually, in the interest of stating the problem and not the
>>> solution, what we need is a way to add FKs that doesn't lock
>>> everything up to perform the key checks.
>>>
>> Ah, finally a useful comment. I think it might be possible to do an
>> "add FK concurrently" type of command that would take exclusive lock
>> for just long enough to add the triggers, then scan the tables with just
>> AccessShareLock to see if the existing rows meet the constraint, and
>> if so finally mark the constraint "valid". Meanwhile the constraint
>> would be enforced against newly-added rows by the triggers, so nothing
>> gets missed. You'd still get a small hiccup in system performance
>> from the transient exclusive lock, but nothing like as bad as it is
>> now. Would that solve your problem?
>>
>
> That's good, but it doesn't solve the original user complaint about
> needing to re-run many, many large queries to which we already know the
> answer.
>
>
But we don't know it for dead sure, we only think we do. What if the
data for one or other of the tables is corrupted? We'll end up with data
we believe is consistent but in fact is not, ISTM. If you can somehow
guarantee the integrity of data in both tables then we might be
justified in assuming that the FK constraint will be consistent - that's
why I suggested some sort of checksum mechanism might serve the purpose.
cheers
andrew
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:33 Tom Lane <tgl@sss.pgh.pa.us>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 1 reply; 40+ messages in thread
From: Tom Lane @ 2008-06-09 15:33 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Simon Riggs <simon@2ndquadrant.com> writes:
> On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
>> Ah, finally a useful comment. I think it might be possible to do an
>> "add FK concurrently" type of command that would take exclusive lock
> That's good, but it doesn't solve the original user complaint about
> needing to re-run many, many large queries to which we already know the
> answer.
No, we are running a large query to which the user *thinks* he knows the
answer. There are any number of reasons why he might be wrong.
regards, tom lane
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:46 Simon Riggs <simon@2ndquadrant.com>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-09 15:46 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
On Mon, 2008-06-09 at 11:23 -0400, Andrew Dunstan wrote:
>
> Simon Riggs wrote:
> > On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
> >
> >> Decibel! <decibel@decibel.org> writes:
> >>
> >>> Actually, in the interest of stating the problem and not the
> >>> solution, what we need is a way to add FKs that doesn't lock
> >>> everything up to perform the key checks.
> >>>
> >> Ah, finally a useful comment. I think it might be possible to do an
> >> "add FK concurrently" type of command that would take exclusive lock
> >> for just long enough to add the triggers, then scan the tables with just
> >> AccessShareLock to see if the existing rows meet the constraint, and
> >> if so finally mark the constraint "valid". Meanwhile the constraint
> >> would be enforced against newly-added rows by the triggers, so nothing
> >> gets missed. You'd still get a small hiccup in system performance
> >> from the transient exclusive lock, but nothing like as bad as it is
> >> now. Would that solve your problem?
> >>
> >
> > That's good, but it doesn't solve the original user complaint about
> > needing to re-run many, many large queries to which we already know the
> > answer.
> >
> But we don't know it for dead sure, we only think we do. What if the
> data for one or other of the tables is corrupted? We'll end up with data
> we believe is consistent but in fact is not, ISTM. If you can somehow
> guarantee the integrity of data in both tables then we might be
> justified in assuming that the FK constraint will be consistent - that's
> why I suggested some sort of checksum mechanism might serve the purpose.
Agreed.
Can we get COPY to output the checksum of its output as part of the
command tag? How else can we return the checksum? In $file.cksum for any
given output file?
We can then use an explicit checksum option in the COPY when we reload,
with CHECKSUM option.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:48 Simon Riggs <simon@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-09 15:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
On Mon, 2008-06-09 at 11:33 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
> >> Ah, finally a useful comment. I think it might be possible to do an
> >> "add FK concurrently" type of command that would take exclusive lock
>
> > That's good, but it doesn't solve the original user complaint about
> > needing to re-run many, many large queries to which we already know the
> > answer.
>
> No, we are running a large query to which the user *thinks* he knows the
> answer. There are any number of reasons why he might be wrong.
Of course. I should have said "to which we already know the answer" to
indicate I'm passing on others' criticisms of us.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 15:59 Tom Lane <tgl@sss.pgh.pa.us>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 1 reply; 40+ messages in thread
From: Tom Lane @ 2008-06-09 15:59 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Simon Riggs <simon@2ndquadrant.com> writes:
> On Mon, 2008-06-09 at 11:33 -0400, Tom Lane wrote:
>> No, we are running a large query to which the user *thinks* he knows the
>> answer. There are any number of reasons why he might be wrong.
> Of course. I should have said "to which we already know the answer" to
> indicate I'm passing on others' criticisms of us.
[ shrug... ] We don't know the answer either, and anyone who says
we do is merely betraying his ignorance of the number of ways to load
a foot-gun.
I don't have any confidence in the "checksum" proposal either, as it's
still naively assuming that changes in the data are the only possible
problem. Consider that you are loading the data into a new database,
which might be running under a different locale setting, might contain
a new implementation of a datatype with subtly (or not so subtly)
different semantics, or might just already contain data in the target
tables. pg_dump scripts are not nearly bright enough to defend against
these types of threats.
regards, tom lane
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 16:09 =?UTF-8?Q?Filip_Rembia=C5=82kowski?= <plk.zuber@gmail.com>
parent: Simon Riggs <simon@2ndquadrant.com>
2 siblings, 0 replies; 40+ messages in thread
From: =?UTF-8?Q?Filip_Rembia=C5=82kowski?= @ 2008-06-09 16:09 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
2008/6/9 Simon Riggs <simon@2ndquadrant.com>:
>
> On Mon, 2008-06-09 at 10:57 -0400, Tom Lane wrote:
> > Decibel! <decibel@decibel.org> writes:
> > > Actually, in the interest of stating the problem and not the
> > > solution, what we need is a way to add FKs that doesn't lock
> > > everything up to perform the key checks.
> >
> > Ah, finally a useful comment. I think it might be possible to do an
> > "add FK concurrently" type of command that would take exclusive lock
> > for just long enough to add the triggers, then scan the tables with just
> > AccessShareLock to see if the existing rows meet the constraint, and
> > if so finally mark the constraint "valid". Meanwhile the constraint
> > would be enforced against newly-added rows by the triggers, so nothing
> > gets missed. You'd still get a small hiccup in system performance
> > from the transient exclusive lock, but nothing like as bad as it is
> > now. Would that solve your problem?
>
> That's good, but it doesn't solve the original user complaint about
> needing to re-run many, many large queries to which we already know the
> answer.
just a guess, but maybe "create FK concurrently" feature combined with
"synchronized scan" feature _does_ resolve original problem.
if you run many "create FK concurrently" one after another, wouldn't the seq
scan be reused?
>
> --
> Simon Riggs www.2ndQuadrant.com
> PostgreSQL Training, Services and Support
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
--
Filip Rembiałkowski
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 16:10 Andrew Dunstan <andrew@dunslane.net>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Dunstan @ 2008-06-09 16:10 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Robert Treat <xzilla@users.sourceforge.net>; pgsql-hackers
Simon Riggs wrote:
>> But we don't know it for dead sure, we only think we do. What if the
>> data for one or other of the tables is corrupted? We'll end up with data
>> we believe is consistent but in fact is not, ISTM. If you can somehow
>> guarantee the integrity of data in both tables then we might be
>> justified in assuming that the FK constraint will be consistent - that's
>> why I suggested some sort of checksum mechanism might serve the purpose.
>>
>
> Agreed.
>
> Can we get COPY to output the checksum of its output as part of the
> command tag? How else can we return the checksum? In $file.cksum for any
> given output file?
>
It seems a reasonable idea to use the command tag, unless that's going
to break lots of stuff. I think the only thing we can usefully checksum
is the output lines in the client encoding.
> We can then use an explicit checksum option in the COPY when we reload,
> with CHECKSUM option.
>
>
We need rather more than this to make sure your facility isn't abused.
That's the part that I haven't been able to think of a good answer for
(yet).
cheers
andrew
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 16:37 Robert Treat <xzilla@users.sourceforge.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 40+ messages in thread
From: Robert Treat @ 2008-06-09 16:37 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Simon Riggs <simon@2ndquadrant.com>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
On Monday 09 June 2008 11:59:27 Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Mon, 2008-06-09 at 11:33 -0400, Tom Lane wrote:
> >> No, we are running a large query to which the user *thinks* he knows the
> >> answer. There are any number of reasons why he might be wrong.
> >
> > Of course. I should have said "to which we already know the answer" to
> > indicate I'm passing on others' criticisms of us.
>
> [ shrug... ] We don't know the answer either, and anyone who says
> we do is merely betraying his ignorance of the number of ways to load
> a foot-gun.
>
I think the more realistic scenario (based on the FK idea) is that you want to
prevent any future rows from coming without validating the FK, and you're
willing to clean up any violators after the fact, since you can make that
an "out of the critical path" operation.
if you extend this to a more general "create constraint concurrently" (to
handle normal constraint, not null constraints, etc...), it would certainly
be a big win, and i think most would see it as a reasonable compromise.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 17:54 Simon Riggs <simon@2ndquadrant.com>
parent: Robert Treat <xzilla@users.sourceforge.net>
0 siblings, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-09 17:54 UTC (permalink / raw)
To: Robert Treat <xzilla@users.sourceforge.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
On Mon, 2008-06-09 at 12:37 -0400, Robert Treat wrote:
> On Monday 09 June 2008 11:59:27 Tom Lane wrote:
> > Simon Riggs <simon@2ndquadrant.com> writes:
> > > On Mon, 2008-06-09 at 11:33 -0400, Tom Lane wrote:
> > >> No, we are running a large query to which the user *thinks* he knows the
> > >> answer. There are any number of reasons why he might be wrong.
> > >
> > > Of course. I should have said "to which we already know the answer" to
> > > indicate I'm passing on others' criticisms of us.
> >
> > [ shrug... ] We don't know the answer either, and anyone who says
> > we do is merely betraying his ignorance of the number of ways to load
> > a foot-gun.
> >
>
> I think the more realistic scenario (based on the FK idea) is that you want to
> prevent any future rows from coming without validating the FK, and you're
> willing to clean up any violators after the fact, since you can make that
> an "out of the critical path" operation.
>
> if you extend this to a more general "create constraint concurrently" (to
> handle normal constraint, not null constraints, etc...), it would certainly
> be a big win, and i think most would see it as a reasonable compromise.
Agreed. I think the "out of the critical path" action is more likely to
be the intended path rather than the "never check at all" route.
If we break down the action into two parts.
ALTER TABLE ... ADD CONSTRAINT foo FOREIGN KEY ... NOVALIDATE;
which holds exclusive lock, but only momentarily
After this runs any new data is validated at moment of data change, but
the older data has yet to be validated.
ALTER TABLE ... VALIDATE CONSTRAINT foo
which runs lengthy check, though only grabs lock as last part of action
This way we have the ability to add them concurrently if we choose by
running one after the other, or we can run first part only for now and
run the other one at a more convenient moment.
On a full set of checks on a large complex database can easily take
hours or even days.
We should allow this. It's not a footgun, its an honest attempt by
people to add RI checks to their database. The only other alternative
for some people is to not add FKs at all, which is also a footgun, but
we don't seem bothered that they might take that option.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 18:07 Alvaro Herrera <alvherre@commandprompt.com>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 2 replies; 40+ messages in thread
From: Alvaro Herrera @ 2008-06-09 18:07 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Robert Treat <xzilla@users.sourceforge.net>; Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
Simon Riggs wrote:
> If we break down the action into two parts.
>
> ALTER TABLE ... ADD CONSTRAINT foo FOREIGN KEY ... NOVALIDATE;
> which holds exclusive lock, but only momentarily
> After this runs any new data is validated at moment of data change, but
> the older data has yet to be validated.
>
> ALTER TABLE ... VALIDATE CONSTRAINT foo
> which runs lengthy check, though only grabs lock as last part of action
The problem I see with this approach in general (two-phase FK creation)
is that you have to keep the same transaction for the first and second
command, but you really want concurrent backends to see the tuple for
the not-yet-validated constraint row.
Another benefit that could arise from this is that the hypothetical
VALIDATE CONSTRAINT step could validate more than one constraint at a
time, possibly processing all the constraints with a single table scan.
Perhaps VALIDATE CONSTRAINT could be handled as an automatic commit-time
action.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-09 20:52 Gregory Stark <stark@enterprisedb.com>
parent: Alvaro Herrera <alvherre@commandprompt.com>
1 sibling, 0 replies; 40+ messages in thread
From: Gregory Stark @ 2008-06-09 20:52 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@commandprompt.com>; +Cc: Simon Riggs <simon@2ndquadrant.com>; Robert Treat <xzilla@users.sourceforge.net>; Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
"Alvaro Herrera" <alvherre@commandprompt.com> writes:
> The problem I see with this approach in general (two-phase FK creation)
> is that you have to keep the same transaction for the first and second
> command, but you really want concurrent backends to see the tuple for
> the not-yet-validated constraint row.
Do you? It seems like having a constraint which is enforced on any new
operations but which doesn't guarantee that existing records satisfy it is a
useful feature in itself -- separating the two concepts "this property is true
for all records" and "any action taken must leave the record with this
property"
ISTM you can validate an "invalid" constraint using any snapshot taken at any
time >= the original snapshot. As long as the constraint is being enforced for
all transactions which start after the validating snapshot's xmin then when
it's done it can know the constraint is valid.
Taking a lock on the table to create the constraint certainly leaves that
property fulfilled. Actually it seems we could not take any lock and just
check when it comes time to do the validation that the snapshot's xmin is >=
the xmin on the constraint. I'm starting to get leery of all these tightly
argued bits of logic though. Each one on its own is safe but the resulting
system is getting to be quite complex.
> Another benefit that could arise from this is that the hypothetical
> VALIDATE CONSTRAINT step could validate more than one constraint at a
> time, possibly processing all the constraints with a single table scan.
Interesting.
> Perhaps VALIDATE CONSTRAINT could be handled as an automatic commit-time
> action.
I don't really like this, at least not as the only option, because as I said
above and Robert Treat also said, it could be useful to have the constraint in
place for new operations but check it for the existing data at some later
date. (Or even never)
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-10 00:14 Simon Riggs <simon@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@commandprompt.com>
1 sibling, 1 reply; 40+ messages in thread
From: Simon Riggs @ 2008-06-10 00:14 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@commandprompt.com>; +Cc: Robert Treat <xzilla@users.sourceforge.net>; Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
On Mon, 2008-06-09 at 14:07 -0400, Alvaro Herrera wrote:
> Simon Riggs wrote:
>
> > If we break down the action into two parts.
> >
> > ALTER TABLE ... ADD CONSTRAINT foo FOREIGN KEY ... NOVALIDATE;
> > which holds exclusive lock, but only momentarily
> > After this runs any new data is validated at moment of data change, but
> > the older data has yet to be validated.
> >
> > ALTER TABLE ... VALIDATE CONSTRAINT foo
> > which runs lengthy check, though only grabs lock as last part of action
>
> The problem I see with this approach in general (two-phase FK creation)
> is that you have to keep the same transaction for the first and second
> command, but you really want concurrent backends to see the tuple for
> the not-yet-validated constraint row.
Well, they *must* be in separate transactions if we are to avoid holding
an AccessExclusiveLock while we perform the check. Plus the whole idea
is to perform the second part at some other non-critical time, though we
all agree that never performing the check at all is foolhardy.
Maybe we say that you can defer the check, but after a while autovacuum
runs it for you if you haven't done so. It would certainly be useful to
run the VALIDATE part as a background task with vacuum wait enabled.
> Another benefit that could arise from this is that the hypothetical
> VALIDATE CONSTRAINT step could validate more than one constraint at a
> time, possibly processing all the constraints with a single table scan.
Good thought, though not as useful for FK checks.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
^ permalink raw reply [nested|flat] 40+ messages in thread
* Re: pg_dump restore time and Foreign Keys
@ 2008-06-10 00:37 Alvaro Herrera <alvherre@commandprompt.com>
parent: Simon Riggs <simon@2ndquadrant.com>
0 siblings, 0 replies; 40+ messages in thread
From: Alvaro Herrera @ 2008-06-10 00:37 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Robert Treat <xzilla@users.sourceforge.net>; Tom Lane <tgl@sss.pgh.pa.us>; Decibel! <decibel@decibel.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers
Simon Riggs wrote:
> Maybe we say that you can defer the check, but after a while autovacuum
> runs it for you if you haven't done so. It would certainly be useful to
> run the VALIDATE part as a background task with vacuum wait enabled.
It would be useful if there was anywhere to report the error to, or an
action that could be taken automatically.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH 2/7] s!also remove PATH.
@ 2022-02-25 23:23 Justin Pryzby <pryzbyj@telsasoft.com>
0 siblings, 0 replies; 40+ messages in thread
From: Justin Pryzby @ 2022-02-25 23:23 UTC (permalink / raw)
This essentially reverts commit f4ce6c4d3a30ec3a12c7f64b90a6fc82887ddd7b
XXX: also set PATH=bindir ?
---
src/Makefile.global.in | 6 +++---
src/test/perl/PostgreSQL/Test/Utils.pm | 10 ++++++++++
src/tools/msvc/vcregress.pl | 4 ----
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 92649d0193..c0e5661338 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -440,7 +440,7 @@ ld_library_path_var = LD_LIBRARY_PATH
# need something more here. If not defined then the expansion does
# nothing.
with_temp_install = \
- PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" \
$(call add_to_path,$(strip $(ld_library_path_var)),$(abs_top_builddir)/tmp_install$(libdir)) \
$(with_temp_install_extra)
@@ -451,7 +451,7 @@ define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
@@ -462,7 +462,7 @@ define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 0e2abb6c9e..41c373fadd 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -190,6 +190,16 @@ INIT
$ENV{TESTDIR} = $test_dir;
+ if ($PostgreSQL::Test::Utils::windows_os &&
+ $Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} =~ s!;!;$test_dir;!;
+ }
+ else
+ {
+ $ENV{PATH} =~ s!:!:$test_dir:!;
+ }
+
# Determine output directories, and create them.
$tmp_check = "$test_dir/tmp_check";
$log_path = "$tmp_check/log";
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e805a9f8f6..74aa572d2f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -261,10 +261,6 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
-
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.17.1
--ibvzjYYg+QDzMCy1
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0003-cirrus-windows-increase-timeout-to-20min.patch"
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH 04/19] s!also remove PATH.
@ 2022-02-25 23:23 Justin Pryzby <pryzbyj@telsasoft.com>
0 siblings, 0 replies; 40+ messages in thread
From: Justin Pryzby @ 2022-02-25 23:23 UTC (permalink / raw)
This essentially reverts commit f4ce6c4d3a30ec3a12c7f64b90a6fc82887ddd7b
And partially reverts 6b04abdfc5e0653542ac5d586e639185a8c61a39
XXX: also set PATH=bindir ?
---
src/Makefile.global.in | 6 +++---
src/interfaces/libpq/Makefile | 4 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 10 ++++++++++
src/tools/msvc/vcregress.pl | 4 ----
4 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 1e1b44485d5..fd1ba039e61 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -440,7 +440,7 @@ ld_library_path_var = LD_LIBRARY_PATH
# need something more here. If not defined then the expansion does
# nothing.
with_temp_install = \
- PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" \
$(call add_to_path,$(strip $(ld_library_path_var)),$(abs_top_builddir)/tmp_install$(libdir)) \
$(with_temp_install_extra)
@@ -452,7 +452,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
@@ -464,7 +464,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index b5fd72a4acf..c5aa91df3d9 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -144,10 +144,10 @@ test-build:
$(MAKE) -C test all
check: test-build all
- PATH="$(CURDIR)/test:$$PATH" && $(prove_check)
+ $(prove_check)
installcheck: test-build all
- PATH="$(CURDIR)/test:$$PATH" && $(prove_installcheck)
+ $(prove_installcheck)
installdirs: installdirs-lib
$(MKDIR_P) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' '$(DESTDIR)$(datadir)'
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 063a19b1df5..c6c4b2fec8a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -195,6 +195,16 @@ INIT
$ENV{TESTDIR} = $test_dir;
+ if ($PostgreSQL::Test::Utils::windows_os &&
+ $Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} =~ s!;!;$test_dir;!;
+ }
+ else
+ {
+ $ENV{PATH} =~ s!:!:$test_dir:!;
+ }
+
# Determine output directories, and create them.
$tmp_check = "$test_dir/tmp_check";
$log_path = "$tmp_check/log";
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index ae19ff5736b..54c31b0e14f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,10 +291,6 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
-
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.17.1
--Sw7tCqrGA+HQ0/zt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0005-cirrus-macos-enable-various-runtime-checks.patch"
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH 04/19] s!also remove PATH.
@ 2022-02-25 23:23 Justin Pryzby <pryzbyj@telsasoft.com>
0 siblings, 0 replies; 40+ messages in thread
From: Justin Pryzby @ 2022-02-25 23:23 UTC (permalink / raw)
This essentially reverts commit f4ce6c4d3a30ec3a12c7f64b90a6fc82887ddd7b
And partially reverts 6b04abdfc5e0653542ac5d586e639185a8c61a39
XXX: also set PATH=bindir ?
---
src/Makefile.global.in | 6 +++---
src/interfaces/libpq/Makefile | 4 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 10 ++++++++++
src/tools/msvc/vcregress.pl | 4 ----
4 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 1e1b44485d5..fd1ba039e61 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -440,7 +440,7 @@ ld_library_path_var = LD_LIBRARY_PATH
# need something more here. If not defined then the expansion does
# nothing.
with_temp_install = \
- PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" \
$(call add_to_path,$(strip $(ld_library_path_var)),$(abs_top_builddir)/tmp_install$(libdir)) \
$(with_temp_install_extra)
@@ -452,7 +452,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
@@ -464,7 +464,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index b5fd72a4acf..c5aa91df3d9 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -144,10 +144,10 @@ test-build:
$(MAKE) -C test all
check: test-build all
- PATH="$(CURDIR)/test:$$PATH" && $(prove_check)
+ $(prove_check)
installcheck: test-build all
- PATH="$(CURDIR)/test:$$PATH" && $(prove_installcheck)
+ $(prove_installcheck)
installdirs: installdirs-lib
$(MKDIR_P) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' '$(DESTDIR)$(datadir)'
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 063a19b1df5..c6c4b2fec8a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -195,6 +195,16 @@ INIT
$ENV{TESTDIR} = $test_dir;
+ if ($PostgreSQL::Test::Utils::windows_os &&
+ $Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} =~ s!;!;$test_dir;!;
+ }
+ else
+ {
+ $ENV{PATH} =~ s!:!:$test_dir:!;
+ }
+
# Determine output directories, and create them.
$tmp_check = "$test_dir/tmp_check";
$log_path = "$tmp_check/log";
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index ae19ff5736b..54c31b0e14f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,10 +291,6 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
-
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.17.1
--Sw7tCqrGA+HQ0/zt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0005-cirrus-macos-enable-various-runtime-checks.patch"
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH 04/21] s!also remove PATH.
@ 2022-02-25 23:23 Justin Pryzby <pryzbyj@telsasoft.com>
0 siblings, 0 replies; 40+ messages in thread
From: Justin Pryzby @ 2022-02-25 23:23 UTC (permalink / raw)
This essentially reverts commit f4ce6c4d3a30ec3a12c7f64b90a6fc82887ddd7b
NO And partially reverts 6b04abdfc5e0653542ac5d586e639185a8c61a39
XXX: also set PATH=bindir ?
---
src/Makefile.global.in | 6 +++---
src/interfaces/libpq/Makefile | 1 +
src/test/perl/PostgreSQL/Test/Utils.pm | 10 ++++++++++
src/tools/msvc/vcregress.pl | 4 ----
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 1e1b44485d5..fd1ba039e61 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -440,7 +440,7 @@ ld_library_path_var = LD_LIBRARY_PATH
# need something more here. If not defined then the expansion does
# nothing.
with_temp_install = \
- PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" \
$(call add_to_path,$(strip $(ld_library_path_var)),$(abs_top_builddir)/tmp_install$(libdir)) \
$(with_temp_install_extra)
@@ -452,7 +452,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
@@ -464,7 +464,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index b5fd72a4acf..d536d850c3c 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -143,6 +143,7 @@ install: all installdirs install-lib
test-build:
$(MAKE) -C test all
+# XXX
check: test-build all
PATH="$(CURDIR)/test:$$PATH" && $(prove_check)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 063a19b1df5..c6c4b2fec8a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -195,6 +195,16 @@ INIT
$ENV{TESTDIR} = $test_dir;
+ if ($PostgreSQL::Test::Utils::windows_os &&
+ $Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} =~ s!;!;$test_dir;!;
+ }
+ else
+ {
+ $ENV{PATH} =~ s!:!:$test_dir:!;
+ }
+
# Determine output directories, and create them.
$tmp_check = "$test_dir/tmp_check";
$log_path = "$tmp_check/log";
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index ae19ff5736b..54c31b0e14f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,10 +291,6 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
-
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.17.1
--STPqjqpCrtky8aYs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0005-cirrus-macos-enable-various-runtime-checks.patch"
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH 04/21] s!also remove PATH.
@ 2022-02-25 23:23 Justin Pryzby <pryzbyj@telsasoft.com>
0 siblings, 0 replies; 40+ messages in thread
From: Justin Pryzby @ 2022-02-25 23:23 UTC (permalink / raw)
This essentially reverts commit f4ce6c4d3a30ec3a12c7f64b90a6fc82887ddd7b
NO And partially reverts 6b04abdfc5e0653542ac5d586e639185a8c61a39
XXX: also set PATH=bindir ?
---
src/Makefile.global.in | 6 +++---
src/interfaces/libpq/Makefile | 1 +
src/test/perl/PostgreSQL/Test/Utils.pm | 10 ++++++++++
src/tools/msvc/vcregress.pl | 4 ----
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index cff13bb8087..92bf2d20d7c 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -439,7 +439,7 @@ ld_library_path_var = LD_LIBRARY_PATH
# need something more here. If not defined then the expansion does
# nothing.
with_temp_install = \
- PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" \
$(call add_to_path,$(strip $(ld_library_path_var)),$(abs_top_builddir)/tmp_install$(libdir)) \
$(with_temp_install_extra)
@@ -451,7 +451,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
@@ -463,7 +463,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- PATH="$(bindir):$(CURDIR):$$PATH" \
+ PATH="$(bindir):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_SUBDIR='$(CURDIR)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index b5fd72a4acf..d536d850c3c 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -143,6 +143,7 @@ install: all installdirs install-lib
test-build:
$(MAKE) -C test all
+# XXX
check: test-build all
PATH="$(CURDIR)/test:$$PATH" && $(prove_check)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 063a19b1df5..c6c4b2fec8a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -195,6 +195,16 @@ INIT
$ENV{TESTDIR} = $test_dir;
+ if ($PostgreSQL::Test::Utils::windows_os &&
+ $Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} =~ s!;!;$test_dir;!;
+ }
+ else
+ {
+ $ENV{PATH} =~ s!:!:$test_dir:!;
+ }
+
# Determine output directories, and create them.
$tmp_check = "$test_dir/tmp_check";
$log_path = "$tmp_check/log";
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index ae19ff5736b..54c31b0e14f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,10 +291,6 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
-
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.17.1
--mln0rGgUGuXEqmuI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0005-cirrus-macos-enable-various-runtime-checks.patch"
^ permalink raw reply [nested|flat] 40+ messages in thread
* [PATCH] Add hook for plugins to acquire sample rows during ANALYZE
@ 2026-05-29 12:35 Samba Siva <sambasivareddy.ch@zohomail.in>
0 siblings, 0 replies; 40+ messages in thread
From: Samba Siva @ 2026-05-29 12:35 UTC (permalink / raw)
- Introduced AcquireSampleRowsFunc_hook for extensions to override row sampling.
- Updated analyze.c to utilize the hook if registered.
- Added tests to ensure ANALYZE completes without errors with the new hook.
---
doc/src/sgml/xfunc.sgml | 10 ++++++++++
src/backend/commands/analyze.c | 24 ++++++++++++++++++++----
src/include/commands/vacuum.h | 11 +++++++++++
src/test/regress/expected/analyze.out | 20 ++++++++++++++++++++
src/test/regress/parallel_schedule | 1 +
src/test/regress/sql/analyze.sql | 24 ++++++++++++++++++++++++
6 files changed, 86 insertions(+), 4 deletions(-)
create mode 100644 src/test/regress/expected/analyze.out
create mode 100644 src/test/regress/sql/analyze.sql
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 1eb5abffd8..f1ef025ae2 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -4203,4 +4203,14 @@ supportfn(internal) returns internal
To create such conditions, the support function must implement
the <literal>SupportRequestIndexCondition</literal> request type.
</para>
+
+ <para>
+ The hook variable <varname>AcquireSampleRowsFunc_hook</varname> allows extensions
+ to override the row sampling function during <command>ANALYZE</command> for regular
+ heap relations. This is useful for extensions/systems which implement distributed
+ databases and want to sample rows from remote nodes instead of the local heap.
+ The hook function fills the provided <literal>rows[]</literal> buffer with at most
+ <literal>targrows</literal> heap tuples and sets <literal>*totalrows</literal> to the
+ estimated total live row count of the relation.
+ </para>
</sect1>
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 4fffb76e55..3560acdff1 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -74,6 +74,8 @@ int default_statistics_target = 100;
static MemoryContext anl_context = NULL;
static BufferAccessStrategy vac_strategy;
+/* Hook for plugins to acquire sample rows for ANALYZE */
+AcquireSampleRowsFunc_hook_type AcquireSampleRowsFunc_hook = NULL;
static void do_analyze_rel(Relation onerel,
VacuumParams *params, List *va_cols,
@@ -188,8 +190,15 @@ analyze_rel(Oid relid, RangeVar *relation,
if (onerel->rd_rel->relkind == RELKIND_RELATION ||
onerel->rd_rel->relkind == RELKIND_MATVIEW)
{
- /* Regular table, so we'll use the regular row acquisition function */
- acquirefunc = acquire_sample_rows;
+ /*
+ * Regular table, so we'll use the regular row acquisition function.
+ * If a plugin has registered a hook to acquire sample rows, use it;
+ * otherwise use the default function.
+ */
+ if (AcquireSampleRowsFunc_hook)
+ acquirefunc = AcquireSampleRowsFunc_hook;
+ else
+ acquirefunc = acquire_sample_rows;
/* Also get regular table's size */
relpages = RelationGetNumberOfBlocks(onerel);
}
@@ -1467,8 +1476,15 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
if (childrel->rd_rel->relkind == RELKIND_RELATION ||
childrel->rd_rel->relkind == RELKIND_MATVIEW)
{
- /* Regular table, so use the regular row acquisition function */
- acquirefunc = acquire_sample_rows;
+ /*
+ * Regular table, so use the regular row acquisition function.
+ * If a plugin has registered a hook to acquire sample rows, use it;
+ * otherwise use the default function.
+ */
+ if (AcquireSampleRowsFunc_hook)
+ acquirefunc = AcquireSampleRowsFunc_hook;
+ else
+ acquirefunc = acquire_sample_rows;
relpages = RelationGetNumberOfBlocks(childrel);
}
else if (childrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bc37a80dc7..146f936861 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -21,6 +21,7 @@
#include "catalog/pg_class.h"
#include "catalog/pg_statistic.h"
#include "catalog/pg_type.h"
+#include "foreign/fdwapi.h"
#include "parser/parse_node.h"
#include "storage/buf.h"
#include "storage/lock.h"
@@ -113,6 +114,9 @@ typedef void (*AnalyzeAttrComputeStatsFunc) (VacAttrStatsP stats,
int samplerows,
double totalrows);
+/* Hook type for plugins to acquire sample rows for ANALYZE */
+typedef AcquireSampleRowsFunc AcquireSampleRowsFunc_hook_type;
+
typedef struct VacAttrStats
{
/*
@@ -334,6 +338,13 @@ extern PGDLLIMPORT int vacuum_cost_limit;
extern PGDLLIMPORT int64 parallel_vacuum_worker_delay_ns;
+/*
+ * Hook for plugins to override row sampling during ANALYZE.
+ * Also applies to child relations of partitioned/inherited tables.
+ * See acquire_sample_rows() in src/backend/commands/analyze.c.
+ */
+extern PGDLLIMPORT AcquireSampleRowsFunc_hook_type AcquireSampleRowsFunc_hook;
+
/* in commands/vacuum.c */
extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
extern void vacuum(List *relations, VacuumParams *params,
diff --git a/src/test/regress/expected/analyze.out b/src/test/regress/expected/analyze.out
new file mode 100644
index 0000000000..5bb6bbc45b
--- /dev/null
+++ b/src/test/regress/expected/analyze.out
@@ -0,0 +1,20 @@
+-- Test AcquireSampleRowsFunc_hook
+-- Usually this would be tested via a C extension.
+-- Here we just confirm this does not break the existing ANALYZE code
+-- by verifying that ANALYZE completes without error.
+CREATE TABLE employees (
+ id SERIAL PRIMARY KEY,
+ name TEXT,
+ department TEXT,
+ salary NUMERIC
+);
+INSERT INTO employees
+ SELECT
+ i,
+ 'Employee ' || i,
+ 'Department ' || (i % 5),
+ (i % 100) * 1000 + 50000
+ FROM generate_series(1, 1000) i;
+-- Should complete without error
+ANALYZE employees;
+DROP TABLE employees;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index e1e0c54019..71978ef858 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -94,6 +94,7 @@ test: vacuum_parallel
# Run this alone, because concurrent DROP TABLE would make non-superuser
# "ANALYZE;" fail with "relation with OID $n does not exist".
test: maintain_every
+test: analyze
# no relation related tests can be put in this group
test: publication subscription
diff --git a/src/test/regress/sql/analyze.sql b/src/test/regress/sql/analyze.sql
new file mode 100644
index 0000000000..e0bc84e9f9
--- /dev/null
+++ b/src/test/regress/sql/analyze.sql
@@ -0,0 +1,24 @@
+-- Test AcquireSampleRowsFunc_hook
+-- Usually this would be tested via a C extension.
+-- Here we just confirm this does not break the existing ANALYZE code
+-- by verifying that ANALYZE completes without error.
+
+CREATE TABLE employees (
+ id SERIAL PRIMARY KEY,
+ name TEXT,
+ department TEXT,
+ salary NUMERIC
+);
+
+INSERT INTO employees
+ SELECT
+ i,
+ 'Employee ' || i,
+ 'Department ' || (i % 5),
+ (i % 100) * 1000 + 50000
+ FROM generate_series(1, 1000) i;
+
+-- Should complete without error
+ANALYZE employees;
+
+DROP TABLE employees;
--
2.50.1 (Apple Git-155)
------=_Part_741589_445982825.1782438414800--
^ permalink raw reply [nested|flat] 40+ messages in thread
end of thread, other threads:[~2026-05-29 12:35 UTC | newest]
Thread overview: 40+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2008-06-05 06:23 pg_dump restore time and Foreign Keys Simon Riggs <simon@2ndquadrant.com>
2008-06-05 07:19 ` Heikki Linnakangas <heikki@enterprisedb.com>
2008-06-05 07:45 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-05 12:23 ` Heikki Linnakangas <heikki@enterprisedb.com>
2008-06-05 13:01 ` Heikki Linnakangas <heikki@enterprisedb.com>
2008-06-05 13:27 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-05 13:35 ` Heikki Linnakangas <heikki@enterprisedb.com>
2008-06-05 08:54 ` Richard Huxton <dev@archonet.com>
2008-06-05 11:57 ` Andrew Dunstan <andrew@dunslane.net>
2008-06-05 12:56 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-07 17:08 ` Robert Treat <xzilla@users.sourceforge.net>
2008-06-07 17:41 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-07 19:00 ` Andrew Dunstan <andrew@dunslane.net>
2008-06-09 12:43 ` Decibel! <decibel@decibel.org>
2008-06-09 14:00 ` Andrew Dunstan <andrew@dunslane.net>
2008-06-09 14:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2008-06-09 15:12 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-09 15:23 ` Andrew Dunstan <andrew@dunslane.net>
2008-06-09 15:46 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-09 16:10 ` Andrew Dunstan <andrew@dunslane.net>
2008-06-09 15:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2008-06-09 15:48 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-09 15:59 ` Tom Lane <tgl@sss.pgh.pa.us>
2008-06-09 16:37 ` Robert Treat <xzilla@users.sourceforge.net>
2008-06-09 17:54 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-09 18:07 ` Alvaro Herrera <alvherre@commandprompt.com>
2008-06-09 20:52 ` Gregory Stark <stark@enterprisedb.com>
2008-06-10 00:14 ` Simon Riggs <simon@2ndquadrant.com>
2008-06-10 00:37 ` Alvaro Herrera <alvherre@commandprompt.com>
2008-06-09 16:09 ` =?UTF-8?Q?Filip_Rembia=C5=82kowski?= <plk.zuber@gmail.com>
2008-06-07 20:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2008-06-08 23:45 ` Robert Treat <xzilla@users.sourceforge.net>
2008-06-09 00:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2008-06-09 00:40 ` Robert Treat <xzilla@users.sourceforge.net>
2022-02-25 23:23 [PATCH 04/19] s!also remove PATH. Justin Pryzby <pryzbyj@telsasoft.com>
2022-02-25 23:23 [PATCH 2/7] s!also remove PATH. Justin Pryzby <pryzbyj@telsasoft.com>
2022-02-25 23:23 [PATCH 04/21] s!also remove PATH. Justin Pryzby <pryzbyj@telsasoft.com>
2022-02-25 23:23 [PATCH 04/19] s!also remove PATH. Justin Pryzby <pryzbyj@telsasoft.com>
2022-02-25 23:23 [PATCH 04/21] s!also remove PATH. Justin Pryzby <pryzbyj@telsasoft.com>
2026-05-29 12:35 [PATCH] Add hook for plugins to acquire sample rows during ANALYZE Samba Siva <sambasivareddy.ch@zohomail.in>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox