Thread: pgsql: Remove ineffective check against dropped columns fromslot_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: https://postgr.es/m/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