Re: segmentation fault using currtid and partitioned tables

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Jaime Casanova <jaime(dot)casanova(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: segmentation fault using currtid and partitioned tables
Date: 2020-04-09 06:22:52
Message-ID: 20200409062252.GP1606@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Apr 08, 2020 at 04:13:31PM +0900, Michael Paquier wrote:
> I have been looking at the tree and the use of the table AM APIs, and
> those TID lookups are really a particular case compared to the other
> callers of the table AM callbacks. Anyway, I have not spotted similar
> problems, so I find very tempting the option to just add some
> RELKIND_HAS_STORAGE() to tid.c where it matters and call it a day.

Playing more with this stuff, it happens that we have zero code
coverage for currtid() and currtid2(), and the main user of those
functions I can find around is the ODBC driver:
https://coverage.postgresql.org/src/backend/utils/adt/tid.c.gcov.html

There are multiple cases to consider, particularly for views:
- Case of a view with ctid as attribute taken from table.
- Case of a view with ctid as attribute with incorrect attribute
type.
It is worth noting that all those code paths can trigger various
elog() errors, which is not something that a user should be able to do
using a SQL-callable function. There are also two code paths for
cases where a view has no or more-than-one SELECT rules, which cannot
normally be reached.

All in that, I propose something like the attached to patch the
surroundings with tests to cover everything I could think of, which I
guess had better be backpatched? While on it, I have noticed that we
lack coverage for max(tid) and min(tid), so I have included a bonus
test.

Another issue is that we don't have any documentation for those
functions, in which case the best fit is a subsection for TID
operators under "Functions and Operators"?

For now, I am adding a patch to next CF so as we don't forget about
this set of issues. Any thoughts?
--
Michael

Attachment Content-Type Size
tid-funcs-fixes-v1.patch text/x-diff 12.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2020-04-09 06:23:48 Re: Vacuum o/p with (full 1, parallel 0) option throwing an error
Previous Message Julien Rouhaud 2020-04-09 06:17:53 Re: Commitfest 2020-03 Now in Progress