Обсуждение: 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.kozhemyakin@postgrespro.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@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      17570
> Logged by:          Alexander Kozhemyakin
> Email address:      a.kozhemyakin@postgrespro.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, 04 Aug 2022 at 19:25, Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Aug 4, 2022 at 6:37 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
>>
>> On Thu, Aug 4, 2022 at 4:28 PM PG Bug reporting form <
>> noreply@postgresql.org> wrote:
>>
>>> The following bug has been logged on the website:
>>>
>>> Bug reference:      17570
>>> Logged by:          Alexander Kozhemyakin
>>> Email address:      a.kozhemyakin@postgrespro.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@hotmail.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

Richard Guo <guofenglinux@gmail.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