get_constraint_index() and conindid
get_constraint_index() does its work by going through pg_depend. It was
added before pg_constraint.conindid was added, and some callers are
still not changed. Are there reasons for that? Probably not. The
attached patch changes get_constraint_index() to an lsyscache-style
lookup instead.
The nearby get_index_constraint() should probably also be changed to
scan pg_constraint instead of pg_depend, but that doesn't have a
syscache to use, so it would be a different approach, so I figured I'd
ask about get_constraint_index() first.
Attachments:
0001-Change-get_constraint_index-to-use-pg_constraint.con.patchtext/plain; charset=UTF-8; name=0001-Change-get_constraint_index-to-use-pg_constraint.con.patch; x-mac-creator=0; x-mac-type=0Download+29-76
On Mon, 7 Dec 2020 at 11:09, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
get_constraint_index() does its work by going through pg_depend. It was
added before pg_constraint.conindid was added, and some callers are
still not changed. Are there reasons for that? Probably not. The
attached patch changes get_constraint_index() to an lsyscache-style
lookup instead.
This looks quite reasonable, and it passes "make installcheck-world".
Only thing I could think of is that it maybe could use a (small)
comment in the message on that/why get_constraint_index is moved to
utils/lsyscache from catalog/dependency, as that took me some time to
understand.
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
On Mon, 7 Dec 2020 at 11:09, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:get_constraint_index() does its work by going through pg_depend. It was
added before pg_constraint.conindid was added, and some callers are
still not changed. Are there reasons for that? Probably not. The
attached patch changes get_constraint_index() to an lsyscache-style
lookup instead.
This looks quite reasonable, and it passes "make installcheck-world".
+1, LGTM.
Only thing I could think of is that it maybe could use a (small)
comment in the message on that/why get_constraint_index is moved to
utils/lsyscache from catalog/dependency, as that took me some time to
understand.
commit message could reasonably say that maybe, but I don't think we
need to memorialize it in a comment. lsyscache.c *is* where one
would expect to find a simple catalog-field-fetch function like this.
The previous implementation was not that, so it didn't belong there.
regards, tom lane
On Tue, Dec 08, 2020 at 01:28:39PM -0500, Tom Lane wrote:
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
On Mon, 7 Dec 2020 at 11:09, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:get_constraint_index() does its work by going through pg_depend. It was
added before pg_constraint.conindid was added, and some callers are
still not changed. Are there reasons for that? Probably not. The
attached patch changes get_constraint_index() to an lsyscache-style
lookup instead.This looks quite reasonable, and it passes "make installcheck-world".
+1, LGTM.
Nice cleanup!
Only thing I could think of is that it maybe could use a (small)
comment in the message on that/why get_constraint_index is moved to
utils/lsyscache from catalog/dependency, as that took me some time to
understand.commit message could reasonably say that maybe, but I don't think we
need to memorialize it in a comment. lsyscache.c *is* where one
would expect to find a simple catalog-field-fetch function like this.
The previous implementation was not that, so it didn't belong there.
Agreed.
--
Michael
On 2020-12-09 07:37, Michael Paquier wrote:
Only thing I could think of is that it maybe could use a (small)
comment in the message on that/why get_constraint_index is moved to
utils/lsyscache from catalog/dependency, as that took me some time to
understand.commit message could reasonably say that maybe, but I don't think we
need to memorialize it in a comment. lsyscache.c *is* where one
would expect to find a simple catalog-field-fetch function like this.
The previous implementation was not that, so it didn't belong there.Agreed.
Thanks, I committed it with an expanded commit message.
After further inspection, I'm not going to do anything about the nearby
get_index_constraint() at this item. The current implementation can use
an index on pg_depend. A scan of pg_constraint has no index available.