pgsql: Remove ineffective check against dropped columns from slot_getat
Remove ineffective check against dropped columns from slot_getattr().
Before this commit slot_getattr() checked for dropped
columns (returning NULL in that case), but only after checking for
previously deformed columns. As slot_deform_tuple() does not contain
such a check, the check in slot_getattr() would often not have been
reached, depending on previous use of the slot.
These days locking and plan invalidation ought to ensure that dropped
columns are not accessed in query plans. Therefore this commit just
drops the insufficient check in slot_getattr(). It's possible that
we'll find some holes againt use of dropped columns, but if so, those
need to be addressed independent of slot_getattr(), as most accesses
don't go through that function anyway.
Author: Andres Freund
Discussion: /messages/by-id/20181107174403.zai7fedgcjoqx44p@alap3.anarazel.de
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/c670d0faace6184216c349a4cf830aa415c58068
Modified Files
--------------
src/backend/executor/execTuples.c | 11 -----------
1 file changed, 11 deletions(-)
On 10 November 2018 at 14:41, Andres Freund <andres@anarazel.de> wrote:
These days locking and plan invalidation ought to ensure that dropped
columns are not accessed in query plans. Therefore this commit just
drops the insufficient check in slot_getattr(). It's possible that
we'll find some holes againt use of dropped columns, but if so, those
need to be addressed independent of slot_getattr(), as most accesses
don't go through that function anyway.
Would it not be worth an Assert(!TupleDescAttr(tupleDesc, attnum -
1)->attisdropped); so that we're more likely to discover any issues
where cached plans are not invalidated correctly?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi,
On 2018-11-11 01:11:34 +1300, David Rowley wrote:
On 10 November 2018 at 14:41, Andres Freund <andres@anarazel.de> wrote:
These days locking and plan invalidation ought to ensure that dropped
columns are not accessed in query plans. Therefore this commit just
drops the insufficient check in slot_getattr(). It's possible that
we'll find some holes againt use of dropped columns, but if so, those
need to be addressed independent of slot_getattr(), as most accesses
don't go through that function anyway.Would it not be worth an Assert(!TupleDescAttr(tupleDesc, attnum -
1)->attisdropped); so that we're more likely to discover any issues
where cached plans are not invalidated correctly?
I don't think it'd really do much. Only very few reads of tuples though
through slot_getattr(). The expression eval machinery - which will be
used for those cases - does verify this on the first execution of an
expression. C.f. CheckVarSlotCompatibility().
If you, or somebody else, feels strongly, we can add one, but I don't
really see the point here.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-11-11 01:11:34 +1300, David Rowley wrote:
Would it not be worth an Assert(!TupleDescAttr(tupleDesc, attnum -
1)->attisdropped); so that we're more likely to discover any issues
where cached plans are not invalidated correctly?
I don't think it'd really do much.
FWIW, I agree. We'd need assertions in many more places than this
if we wanted reasonable coverage on the point, and I doubt it's
worth the work.
regards, tom lane