Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1rDtPW-003UKx-LA
	for pgsql-hackers@arkaria.postgresql.org; Thu, 14 Dec 2023 21:38:26 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1rDtPS-00CCQX-Rg
	for pgsql-hackers@arkaria.postgresql.org; Thu, 14 Dec 2023 21:38:22 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1rDtPS-00CCQP-Hr
	for pgsql-hackers@lists.postgresql.org; Thu, 14 Dec 2023 21:38:22 +0000
Received: from sss.pgh.pa.us ([68.162.161.243])
	by makus.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1rDtPP-00AQMn-BZ
	for pgsql-hackers@postgresql.org; Thu, 14 Dec 2023 21:38:20 +0000
Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1])
	by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 3BELcHLj2827927;
	Thu, 14 Dec 2023 16:38:17 -0500
From: Tom Lane <tgl@sss.pgh.pa.us>
To: James Coleman <jtc331@gmail.com>
cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Teach predtest about IS [NOT] <boolean> proofs
In-reply-to: <CAAaqYe-CGx7sEm1QRNaY=Pfti4KinLOPtgVr4LKoXY6wLa68Ag@mail.gmail.com>
References: <CAAaqYe8Bo4bf_i6qKj8KBsmHMYXhe3Xt6vOe3OBQnOaf3_XBWg@mail.gmail.com> <2641885.1702492574@sss.pgh.pa.us> <CAAaqYe-CGx7sEm1QRNaY=Pfti4KinLOPtgVr4LKoXY6wLa68Ag@mail.gmail.com>
Comments: In-reply-to James Coleman <jtc331@gmail.com>
	message dated "Wed, 13 Dec 2023 19:35:01 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <2827925.1702589897.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 14 Dec 2023 16:38:17 -0500
Message-ID: <2827926.1702589897@sss.pgh.pa.us>
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/2827926.1702589897%40sss.pgh.pa.us>
Precedence: bulk

James Coleman <jtc331@gmail.com> writes:
> On Wed, Dec 13, 2023 at 1:36=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wro=
te:
>> I don't have an objection in principle to adding more smarts to
>> predtest.c.  However, we should be wary of slowing down cases where
>> no BooleanTests are present to be optimized.  I wonder if it could
>> help to use a switch on nodeTag rather than a series of if(IsA())
>> tests.  (I'd be inclined to rewrite the inner if-then-else chains
>> as switches too, really.  You get some benefit from the compiler
>> noticing whether you've covered all the enum values.)

> I think I could take this on; would you prefer it as a patch in this
> series? Or as a new patch thread?

No, keep it in the same thread (and make a CF entry, if you didn't
already).  It might be best to make a series of 2 patches, first
just refactoring what's there per this discussion, and then a
second one to add BooleanTest logic.

>> I note you've actively broken the function's ability to cope with
>> NULL input pointers.  Maybe we don't need it to, but I'm not going
>> to accept a patch that just side-swipes that case without any
>> justification.

> [ all callers have previously used predicate_classify ]

OK, fair enough.  The checks for nulls are probably from ancient
habit, but I agree we could remove 'em here.

>> Perhaps, rather than hoping people will notice comments that are
>> potentially offscreen from what they're modifying, we should relocate
>> those comment paras to be adjacent to the relevant parts of the
>> function?

> Splitting up that block comment makes sense to me.

Done, let's make it so.

>> I've not gone through the patch in detail to see whether I believe
>> the proposed proof rules.  It would help to have more comments
>> justifying them.

> Most of them are sufficiently simple -- e.g., X IS TRUE implies X --
> that I don't think there's a lot to say in justification. In some
> cases I've noted the cases that force only strong or weak implication.

Yeah, it's the strong-vs-weak distinction that makes me cautious here.
One's high-school-algebra instinct for what's obviously true tends to
not think about NULL/UNKNOWN, and you do have to consider that.

>>> As noted in a TODO in the patch itself, I think it may be worth refact=
oring
>>> the test_predtest module to run the "x, y" case as well as the "y, x" =
case

>> I think that's actively undesirable.  It is not typically the case that
>> a proof rule for A =3D> B also works in the other direction, so this wo=
uld
>> encourage wasting cycles in the tests.  I fear it might also cause
>> confusion about which direction a proof rule is supposed to work in.

> That makes sense in the general case.

> Boolean expressions seem like a special case in that regard: (subject
> to what it looks like) would you be OK with a wrapping function that
> does both directions (with output that shows which direction is being
> tested) used only for the cases where we do want to check both
> directions?

Using a wrapper where appropriate would remove the inefficiency
concern, but I still worry whether it will promote confusion about
which direction we're proving things in.  You'll need to be very clear
about the labeling.

			regards, tom lane