Re: BUG #17570: Unrecognized node type for query with statistics on expressions

The following bug has been logged on the website:

Bug reference: 17570
Logged by: Alexander Kozhemyakin
Email address: a(dot)kozhemyakin(at)postgrespro(dot)ru
PostgreSQL version: 14.4
Operating system: ubunu 20.04
Description:

When executing the following script:
CREATE TABLE t(a INT, b VARCHAR);
INSERT INTO t(a, b) SELECT i, i FROM generate_series(1,100) s(i);
CREATE STATISTICS t_stats (mcv) ON (a), (b) FROM t;
ANALYZE t;
CREATE ROLE u;
--GRANT SELECT ON t TO u;
SET SESSION AUTHORIZATION u;
SELECT * FROM t WHERE a = 1 AND b::int = 1;

I get:
ERROR: unrecognized node type: 1697192808

And it fails with error "permission denied for table t" when condition is
just "a = 1" or "b::int = 1".

The same query executed successfully, if the select permission is granted to
user.
The first bad commit is a4d75c86b

On Thu, 04 Aug 2022 at 16:27, PG Bug reporting form <noreply(at)postgresql(dot)org> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 17570
> Logged by: Alexander Kozhemyakin
> Email address: a(dot)kozhemyakin(at)postgrespro(dot)ru
> PostgreSQL version: 14.4
> Operating system: ubunu 20.04
> Description:
>
> When executing the following script:
> CREATE TABLE t(a INT, b VARCHAR);
> INSERT INTO t(a, b) SELECT i, i FROM generate_series(1,100) s(i);
> CREATE STATISTICS t_stats (mcv) ON (a), (b) FROM t;
> ANALYZE t;
> CREATE ROLE u;
> --GRANT SELECT ON t TO u;
> SET SESSION AUTHORIZATION u;
> SELECT * FROM t WHERE a = 1 AND b::int = 1;
>
> I get:
> ERROR: unrecognized node type: 1697192808
>
> And it fails with error "permission denied for table t" when condition is
> just "a = 1" or "b::int = 1".
>
> The same query executed successfully, if the select permission is granted to
> user.
> The first bad commit is a4d75c86b

Yeah, it is a bug.

When we don't have table privilege, we check individual columns, however, it
passes a wrong pointer to pull_varattnos(), which leads to this error.

There has another bug when checking the attribute privilege, since the bitmap
contains the offset by FirstLowInvalidHeapAttributeNumber.

Attach a patch to fix it. Any thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Aug 4, 2022 at 4:28 PM PG Bug reporting form <noreply(at)postgresql(dot)org>
wrote:

> The following bug has been logged on the website:
>
> Bug reference: 17570
> Logged by: Alexander Kozhemyakin
> Email address: a(dot)kozhemyakin(at)postgrespro(dot)ru
> PostgreSQL version: 14.4
> Operating system: ubunu 20.04
> Description:
>
> When executing the following script:
> CREATE TABLE t(a INT, b VARCHAR);
> INSERT INTO t(a, b) SELECT i, i FROM generate_series(1,100) s(i);
> CREATE STATISTICS t_stats (mcv) ON (a), (b) FROM t;
> ANALYZE t;
> CREATE ROLE u;
> --GRANT SELECT ON t TO u;
> SET SESSION AUTHORIZATION u;
> SELECT * FROM t WHERE a = 1 AND b::int = 1;
>
> I get:
> ERROR: unrecognized node type: 1697192808

Thanks for the report! I can reproduce it in HEAD. Propose the attached
for fix.

Thanks
Richard

On Thu, Aug 4, 2022 at 6:37 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

>
> On Thu, Aug 4, 2022 at 4:28 PM PG Bug reporting form <
> noreply(at)postgresql(dot)org> wrote:
>
>> The following bug has been logged on the website:
>>
>> Bug reference: 17570
>> Logged by: Alexander Kozhemyakin
>> Email address: a(dot)kozhemyakin(at)postgrespro(dot)ru
>> PostgreSQL version: 14.4
>> Operating system: ubunu 20.04
>> Description:
>>
>> When executing the following script:
>> CREATE TABLE t(a INT, b VARCHAR);
>> INSERT INTO t(a, b) SELECT i, i FROM generate_series(1,100) s(i);
>> CREATE STATISTICS t_stats (mcv) ON (a), (b) FROM t;
>> ANALYZE t;
>> CREATE ROLE u;
>> --GRANT SELECT ON t TO u;
>> SET SESSION AUTHORIZATION u;
>> SELECT * FROM t WHERE a = 1 AND b::int = 1;
>>
>> I get:
>> ERROR: unrecognized node type: 1697192808
>
>
> Thanks for the report! I can reproduce it in HEAD. Propose the attached
> for fix.
>

Oops.. sorry I didn't notice Japin's email before I hit the 'send' for
my reply. The two patches are almost the same, except that before we
check against whole-row reference, I also adjust the attnum by
FirstLowInvalidHeapAttributeNumber.

Thanks
Richard

On Thu, 04 Aug 2022 at 19:25, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Thu, Aug 4, 2022 at 6:37 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>>
>> On Thu, Aug 4, 2022 at 4:28 PM PG Bug reporting form <
>> noreply(at)postgresql(dot)org> wrote:
>>
>>> The following bug has been logged on the website:
>>>
>>> Bug reference: 17570
>>> Logged by: Alexander Kozhemyakin
>>> Email address: a(dot)kozhemyakin(at)postgrespro(dot)ru
>>> PostgreSQL version: 14.4
>>> Operating system: ubunu 20.04
>>> Description:
>>>
>>> When executing the following script:
>>> CREATE TABLE t(a INT, b VARCHAR);
>>> INSERT INTO t(a, b) SELECT i, i FROM generate_series(1,100) s(i);
>>> CREATE STATISTICS t_stats (mcv) ON (a), (b) FROM t;
>>> ANALYZE t;
>>> CREATE ROLE u;
>>> --GRANT SELECT ON t TO u;
>>> SET SESSION AUTHORIZATION u;
>>> SELECT * FROM t WHERE a = 1 AND b::int = 1;
>>>
>>> I get:
>>> ERROR: unrecognized node type: 1697192808
>>
>>
>> Thanks for the report! I can reproduce it in HEAD. Propose the attached
>> for fix.
>>
>
> Oops.. sorry I didn't notice Japin's email before I hit the 'send' for
> my reply. The two patches are almost the same, except that before we
> check against whole-row reference, I also adjust the attnum by
> FirstLowInvalidHeapAttributeNumber.
>

Doesn't matter.

Attached to new patch add a test-case from Alexander Kozhemyakin.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli(at)hotmail(dot)com> writes:
> Attached to new patch add a test-case from Alexander Kozhemyakin.

It appears to me that this is actually very thoroughly broken.

* statext_is_compatible_clause_internal is not on board with
offsetting the attnums.

* Neither is this check for a whole-row reference (line 1628, in HEAD):

if (bms_is_member(InvalidAttrNumber, clause_attnums))

* Although the coverage report claims this code is exercised, the
fact that nothing failed when you made only a partial fix says it's
not exercised well enough.

* The comments for statext_is_compatible_clause suck. If they'd
defined what the arguments are, perhaps this mess would have been
prevented. For extra credit, it'd be really nice to define what
"is compatible" means. I'd sure not have thought that that would
include permissions checks.

regards, tom lane

On Thu, Aug 4, 2022 at 11:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> * statext_is_compatible_clause_internal is not on board with
> offsetting the attnums.

Correct. Good catch! Fix this in v2 patch.

> * Neither is this check for a whole-row reference (line 1628, in HEAD):
>
> if (bms_is_member(InvalidAttrNumber, clause_attnums))

Actually this has been fixed in my v1 patch [1].

> * Although the coverage report claims this code is exercised, the
> fact that nothing failed when you made only a partial fix says it's
> not exercised well enough.

That's true. We need to provide a better test case to cover this.

>
> * The comments for statext_is_compatible_clause suck. If they'd
> defined what the arguments are, perhaps this mess would have been
> prevented. For extra credit, it'd be really nice to define what
> "is compatible" means. I'd sure not have thought that that would
> include permissions checks.
>

Concur with that.

[1]:
/message-id/CAMbWs4_PPrJ7JFXqewQfkMLP6-2kHi%2B5%3D11U0csQVTx7iq7esw%40mail.gmail.com

Thanks
Richard

On Thu, Aug 4, 2022 at 11:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> * The comments for statext_is_compatible_clause suck. If they'd
> defined what the arguments are, perhaps this mess would have been
> prevented. For extra credit, it'd be really nice to define what
> "is compatible" means. I'd sure not have thought that that would
> include permissions checks.

I think by 'compatible' it just means the clause is in the form that can
be estimated using MCV lists. Maybe we can define it as that.

In statext_is_compatible_clause() we have the permission checks to see
if we are allowed to read all required attributes, which indeed needs to
be mentioned in the function comments.

Thanks
Richard

On Fri, Aug 5, 2022 at 8:49 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

> On Thu, Aug 4, 2022 at 11:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> * statext_is_compatible_clause_internal is not on board with
>> offsetting the attnums.
>
>
> Correct. Good catch! Fix this in v2 patch.
>

I realized the fix in v2 is not correct. The attnums generated by
statext_is_compatible_clause is expected to be NOT offsetting by its
caller, i.e. statext_mcv_clauselist_selectivity. Revise that in v3 patch
by doing the offsetting when composing clause_attnums that need to have
permission check.

> * Although the coverage report claims this code is exercised, the
>> fact that nothing failed when you made only a partial fix says it's
>> not exercised well enough.
>
>
> That's true. We need to provide a better test case to cover this.
>

Also I'm trying to provide a better test case and at last come up with a
case in v3 like:

SELECT * FROM tststats.priv_test_tbl where a = 1 and
tststats.priv_test_tbl.* > (1, 1) is not null;

For this case, the clause 'a = 1' would cover that the attnum comes from
statext_is_compatible_clause_internal. And the clause 'priv_test_tbl.* >
(1, 1) is not null' would cover that the attnum comes from 'exprs' and
it has a whole-row reference.

Thanks
Richard

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> I realized the fix in v2 is not correct. The attnums generated by
> statext_is_compatible_clause is expected to be NOT offsetting by its
> caller, i.e. statext_mcv_clauselist_selectivity. Revise that in v3 patch
> by doing the offsetting when composing clause_attnums that need to have
> permission check.

Yeah, I was afraid that that route would require the patch to metastasize
into more places. Agreed on keeping the argument definition the same
and coping locally instead. But I don't think that you've done enough
to address the root cause of this bug, which is the woeful
under-documentation of the argument. I'll have a go at that part.

regards, tom lane