Horton

Thread

Commits

  1. Change get_constraint_index() to use pg_constraint.conindid

  1. get_constraint_index() and conindid

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-07T10:09:16Z

    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.

  2. Re: get_constraint_index() and conindid

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2020-12-08T14:52:13Z

    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.

  3. Re: get_constraint_index() and conindid

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-08T18:28:39Z

    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

  4. Re: get_constraint_index() and conindid

    Michael Paquier <michael@paquier.xyz> — 2020-12-09T06:37:50Z

    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

  5. Re: get_constraint_index() and conindid

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-09T14:50:26Z

    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.