output columns of \dAo and \dAp

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I'm also wondering whether this is fully correct. Would it be possible for the
argument types of the operator/function to differ from the left arg/right arg
types? (Perhaps binary compatible?)

pg_amop.h specifies that

* The primary key for this table is <amopfamily, amoplefttype, amoprighttype,
* amopstrategy>. amoplefttype and amoprighttype are just copies of the
* operator's oprleft/oprright, ie its declared input data types.

Perhaps it'd be a good idea for opr_sanity.sql to verify that, since
it'd be an easy thing to mess up in handmade pg_amop entries. But
at least for the foreseeable future, there's no reason for \dAo to show
amoplefttype/amoprighttype separately.

I agree that \dAo ought to be showing amopstrategy; moreover that ought
to be much higher in the sort key than it is. Also, if we're not going
to show amoppurpose, then the view probably ought to hide non-search
operators altogether. It is REALLY misleading to not distinguish search
and ordering operators.

The situation is different for pg_amproc: if you look for discrepancies
you will find plenty, since in many cases a support function's signature
has little to do with what types it is registered under. Perhaps it'd be
worthwhile for \dAp to show the functions as regprocedure in addition to
showing amproclefttype/amprocrighttype explicitly. In any case, I think
it's rather misleading for \dAp to label amproclefttype/amprocrighttype as
"Left arg type" and "Right arg type", because for almost everything except
btree/hash, that's not what the support function's arguments actually are.
Perhaps names along the lines of "Registered left type" and "Registered
right type" would put readers in a better frame of mind to understand
the entries.

regards, tom lane

Sergey, Nikita, Alexander, if you can please see this thread and propose
a solution, that'd be very welcome.

On 2020-Jun-06, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I'm also wondering whether this is fully correct. Would it be possible for the
argument types of the operator/function to differ from the left arg/right arg
types? (Perhaps binary compatible?)

pg_amop.h specifies that

* The primary key for this table is <amopfamily, amoplefttype, amoprighttype,
* amopstrategy>. amoplefttype and amoprighttype are just copies of the
* operator's oprleft/oprright, ie its declared input data types.

Perhaps it'd be a good idea for opr_sanity.sql to verify that, since
it'd be an easy thing to mess up in handmade pg_amop entries. But
at least for the foreseeable future, there's no reason for \dAo to show
amoplefttype/amoprighttype separately.

I agree that \dAo ought to be showing amopstrategy; moreover that ought
to be much higher in the sort key than it is. Also, if we're not going
to show amoppurpose, then the view probably ought to hide non-search
operators altogether. It is REALLY misleading to not distinguish search
and ordering operators.

The situation is different for pg_amproc: if you look for discrepancies
you will find plenty, since in many cases a support function's signature
has little to do with what types it is registered under. Perhaps it'd be
worthwhile for \dAp to show the functions as regprocedure in addition to
showing amproclefttype/amprocrighttype explicitly. In any case, I think
it's rather misleading for \dAp to label amproclefttype/amprocrighttype as
"Left arg type" and "Right arg type", because for almost everything except
btree/hash, that's not what the support function's arguments actually are.
Perhaps names along the lines of "Registered left type" and "Registered
right type" would put readers in a better frame of mind to understand
the entries.

regards, tom lane

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Jun 7, 2020 at 12:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I'm also wondering whether this is fully correct. Would it be possible for the
argument types of the operator/function to differ from the left arg/right arg
types? (Perhaps binary compatible?)

pg_amop.h specifies that

* The primary key for this table is <amopfamily, amoplefttype, amoprighttype,
* amopstrategy>. amoplefttype and amoprighttype are just copies of the
* operator's oprleft/oprright, ie its declared input data types.

Perhaps it'd be a good idea for opr_sanity.sql to verify that, since
it'd be an easy thing to mess up in handmade pg_amop entries. But
at least for the foreseeable future, there's no reason for \dAo to show
amoplefttype/amoprighttype separately.

+1 for checking consistency of amoplefttype/amoprighttype in opr_sanity.sql

I agree that \dAo ought to be showing amopstrategy;

I agree that the strategy and purpose of an operator is valuable
information. And we probably shouldn't hide it in \dAo. If we do so,
then \dAo and \dAo+ differ by only "sort opfamily" column. Is it
worth keeping the \dAo+ command for single-column difference?

moreover that ought
to be much higher in the sort key than it is.

Do you mean we should sort by strategy number and only then by
arg types? Current output shows operators grouped by opclasses,
after that cross-opclass operators are shown. This order seems to me
more worthwhile than seeing all the variations of the same strategy
together.

Also, if we're not going
to show amoppurpose, then the view probably ought to hide non-search
operators altogether. It is REALLY misleading to not distinguish search
and ordering operators.

+1

The situation is different for pg_amproc: if you look for discrepancies
you will find plenty, since in many cases a support function's signature
has little to do with what types it is registered under. Perhaps it'd be
worthwhile for \dAp to show the functions as regprocedure in addition to
showing amproclefttype/amprocrighttype explicitly. In any case, I think
it's rather misleading for \dAp to label amproclefttype/amprocrighttype as
"Left arg type" and "Right arg type", because for almost everything except
btree/hash, that's not what the support function's arguments actually are.
Perhaps names along the lines of "Registered left type" and "Registered
right type" would put readers in a better frame of mind to understand
the entries.

+1 for rename "Left arg type"/"Right arg type" to "Registered left
type"/"Registered right type".

------
Regards,
Alexander Korotkov

On 7/7/20 6:09 PM, Alexander Korotkov wrote:

On Sun, Jun 7, 2020 at 12:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I'm also wondering whether this is fully correct. Would it be possible for the
argument types of the operator/function to differ from the left arg/right arg
types? (Perhaps binary compatible?)

pg_amop.h specifies that

* The primary key for this table is <amopfamily, amoplefttype, amoprighttype,
* amopstrategy>. amoplefttype and amoprighttype are just copies of the
* operator's oprleft/oprright, ie its declared input data types.

Perhaps it'd be a good idea for opr_sanity.sql to verify that, since
it'd be an easy thing to mess up in handmade pg_amop entries. But
at least for the foreseeable future, there's no reason for \dAo to show
amoplefttype/amoprighttype separately.

+1 for checking consistency of amoplefttype/amoprighttype in opr_sanity.sql

I agree that \dAo ought to be showing amopstrategy;

I agree that the strategy and purpose of an operator is valuable
information. And we probably shouldn't hide it in \dAo. If we do so,
then \dAo and \dAo+ differ by only "sort opfamily" column. Is it
worth keeping the \dAo+ command for single-column difference?

moreover that ought
to be much higher in the sort key than it is.

Do you mean we should sort by strategy number and only then by
arg types? Current output shows operators grouped by opclasses,
after that cross-opclass operators are shown. This order seems to me
more worthwhile than seeing all the variations of the same strategy
together.

Also, if we're not going
to show amoppurpose, then the view probably ought to hide non-search
operators altogether. It is REALLY misleading to not distinguish search
and ordering operators.

+1

The situation is different for pg_amproc: if you look for discrepancies
you will find plenty, since in many cases a support function's signature
has little to do with what types it is registered under. Perhaps it'd be
worthwhile for \dAp to show the functions as regprocedure in addition to
showing amproclefttype/amprocrighttype explicitly. In any case, I think
it's rather misleading for \dAp to label amproclefttype/amprocrighttype as
"Left arg type" and "Right arg type", because for almost everything except
btree/hash, that's not what the support function's arguments actually are.
Perhaps names along the lines of "Registered left type" and "Registered
right type" would put readers in a better frame of mind to understand
the entries.

+1 for rename "Left arg type"/"Right arg type" to "Registered left
type"/"Registered right type".

From the RMT perspective, if there is an agreed upon approach (which it
sounds like from the above) can someone please commit to working on
resolving this open item?

Thanks!

Jonathan

On Thu, Jul 9, 2020 at 10:03 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

From the RMT perspective, if there is an agreed upon approach (which it
sounds like from the above) can someone please commit to working on
resolving this open item?

I hardly can extract an approach from this thread, because for me the
whole issue is about details :)

But I think we can come to an agreement shortly. And yes, I commit to
resolve this.

------
Regards,
Alexander Korotkov

On Fri, Jul 10, 2020 at 2:24 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Thu, Jul 9, 2020 at 10:03 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

From the RMT perspective, if there is an agreed upon approach (which it
sounds like from the above) can someone please commit to working on
resolving this open item?

I hardly can extract an approach from this thread, because for me the
whole issue is about details :)

But I think we can come to an agreement shortly. And yes, I commit to
resolve this.

The proposed patch is attached. This patch is fixes two points:
* Adds strategy number and purpose to output of \dAo
* Renames "Left/right arg type" columns of \dAp to "Registered left/right type"

I'm not yet convinced we should change the sort key for \dAo.

Any thoughts?

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:

The proposed patch is attached. This patch is fixes two points:
* Adds strategy number and purpose to output of \dAo
* Renames "Left/right arg type" columns of \dAp to "Registered left/right type"

I think that \dAp should additionally be changed to print the
function via "oid::regprocedure", not just proname. A possible
compromise, if you think that's too wordy, is to do it that
way for "\dAp+" while printing plain proname for "\dAp".

BTW, isn't this:

" format ('%%s (%%s, %%s)',\n"
" CASE\n"
" WHEN pg_catalog.pg_operator_is_visible(op.oid) \n"
" THEN op.oprname::pg_catalog.text \n"
" ELSE o.amopopr::pg_catalog.regoper::pg_catalog.text \n"
" END,\n"
" pg_catalog.format_type(o.amoplefttype, NULL),\n"
" pg_catalog.format_type(o.amoprighttype, NULL)\n"
" ) AS \"%s\"\n,"

just an extremely painful way to duplicate the results of regoperator?
(You could likely remove the joins to pg_proc and pg_operator altogether
if you relied on regprocedure and regoperator casts.)

I'm not yet convinced we should change the sort key for \dAo.

After playing with this more, I'm less worried about that than
I was. I think I was concerned that the operator name would
sort ahead of amopstrategy, but now I see that the op name isn't
part of the sort key at all.

BTW, these queries seem inadequately schema-qualified, notably
the format() calls.

regards, tom lane

On Sat, Jul 11, 2020 at 10:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <aekorotkov@gmail.com> writes:

The proposed patch is attached. This patch is fixes two points:
* Adds strategy number and purpose to output of \dAo
* Renames "Left/right arg type" columns of \dAp to "Registered left/right type"

I think that \dAp should additionally be changed to print the
function via "oid::regprocedure", not just proname. A possible
compromise, if you think that's too wordy, is to do it that
way for "\dAp+" while printing plain proname for "\dAp".

Good compromise. Done as you proposed.

BTW, isn't this:

" format ('%%s (%%s, %%s)',\n"
" CASE\n"
" WHEN pg_catalog.pg_operator_is_visible(op.oid) \n"
" THEN op.oprname::pg_catalog.text \n"
" ELSE o.amopopr::pg_catalog.regoper::pg_catalog.text \n"
" END,\n"
" pg_catalog.format_type(o.amoplefttype, NULL),\n"
" pg_catalog.format_type(o.amoprighttype, NULL)\n"
" ) AS \"%s\"\n,"

just an extremely painful way to duplicate the results of regoperator?
(You could likely remove the joins to pg_proc and pg_operator altogether
if you relied on regprocedure and regoperator casts.)

Yeah, this subquery is totally dumb. Replaced with cast to regoperator.

I'm not yet convinced we should change the sort key for \dAo.

After playing with this more, I'm less worried about that than
I was. I think I was concerned that the operator name would
sort ahead of amopstrategy, but now I see that the op name isn't
part of the sort key at all.

Ok.

BTW, these queries seem inadequately schema-qualified, notably
the format() calls.

Thank you for pointing. I've added schema-qualification to pg_catalog
functions and tables.

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:

Good compromise. Done as you proposed.

I'm OK with this version.

regards, tom lane

On 7/13/20 10:37 AM, Tom Lane wrote:

Alexander Korotkov <aekorotkov@gmail.com> writes:

Good compromise. Done as you proposed.

I'm OK with this version.

I saw this was committed and the item was adjusted on the Open Items list.

Thank you!

Jonathan

On Mon, Jul 13, 2020 at 7:54 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

On 7/13/20 10:37 AM, Tom Lane wrote:

Alexander Korotkov <aekorotkov@gmail.com> writes:

Good compromise. Done as you proposed.

I'm OK with this version.

I saw this was committed and the item was adjusted on the Open Items list.

Thank you!

------
Regards,
Alexander Korotkov