Horton

Thread

Commits

  1. Fix use-after-release mistake in currtid() and currtid2() for views

  2. Fix crashes with currtid() and currtid2()

  1. segmentation fault using currtid and partitioned tables

    Jaime Casanova <jaime.casanova@2ndquadrant.com> — 2020-04-05T08:18:50Z

    Hi,

Another one caught by sqlsmith, on the regression database run this
query (using any non-partitioned table works fine):

"""
select currtid('pagg_tab'::regclass::oid, '(0,156)'::tid) >= '(1,158)'::tid;
"""

This works on 11 (well it gives an error because the file doesn't
exists) but crash the server on 12+

attached the stack trace from master

-- 
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: segmentation fault using currtid and partitioned tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-05T16:51:56Z

    Jaime Casanova <jaime.casanova@2ndquadrant.com> writes:
> Another one caught by sqlsmith, on the regression database run this
> query (using any non-partitioned table works fine):
> select currtid('pagg_tab'::regclass::oid, '(0,156)'::tid) >= '(1,158)'::tid;

Hm, so

(1) currtid_byreloid and currtid_byrelname lack any check to see
if they're dealing with a relkind that lacks storage.

(2) The proximate cause of the crash is that rd_tableam is zero,
so that the interface functions in tableam.h just crash hard.
This seems like a pretty bad thing; does anyone want to promise
that there are no other oversights of the same ilk elsewhere,
and never will be?

I think it might be a good idea to make relations-without-storage
set up rd_tableam as a vector of dummy functions that will throw
some suitable complaint about "relation lacks storage".  NULL is
a horrible default for this.

			regards, tom lane

  3. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-04-08T07:13:31Z

    On Sun, Apr 05, 2020 at 12:51:56PM -0400, Tom Lane wrote:
> I think it might be a good idea to make relations-without-storage
> set up rd_tableam as a vector of dummy functions that will throw
> some suitable complaint about "relation lacks storage".  NULL is
> a horrible default for this.

Yeah, that's not good, but I am not really comfortable with the
concept of implying that (pg_class.relam == InvalidOid) maps to a
dummy AM callback set instead of NULL for rd_tableam.  That feels less
natural.  As mentioned upthread, the error that we get in ~11 is
confusing as well when using a relation that has no storage:
ERROR:  58P01: could not open file "base/16384/16385": No such file or directory

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.

Andres, what do you think?
--
Michael

  4. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T06:22:52Z

    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

  5. Re: segmentation fault using currtid and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-22T23:32:57Z

    On 2020-Apr-09, Michael Paquier wrote:

> 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

Yeah, they're there solely for ODBC as far as I know.

> 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?

I don't know, but this stuff is so unused that your patch seems
excessive ... and I think we'd rather not backpatch something so large.
I propose we do something less invasive in the backbranches, like just
throw elog() errors (nothing fancy) where necessary to avoid the
crashes.  Even for pg12 it seems that that should be sufficient.

For pg13 and beyond, I liked Tom's idea of installing dummy functions
for tables without storage -- that seems safer.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-05-25T09:29:10Z

    On Fri, May 22, 2020 at 07:32:57PM -0400, Alvaro Herrera wrote:
> I don't know, but this stuff is so unused that your patch seems
> excessive ... and I think we'd rather not backpatch something so large.
> I propose we do something less invasive in the backbranches, like just
> throw elog() errors (nothing fancy) where necessary to avoid the
> crashes.  Even for pg12 it seems that that should be sufficient.

Even knowing that those trigger a bunch of elog()s which are not
something that should be user-triggerable?  :)

Perhaps you are right though, and that we don't need to spend this
much energy into improving the error messages so I am fine to discard
this part.  At the end, in order to remove the crashes, you just need
to keep around the two RELKIND_HAS_STORAGE() checks.  But I would
rather keep these two to use ereport(ERRCODE_FEATURE_NOT_SUPPORTED)
instead of elog(), and keep the test coverage of the previous patch
(including the tests for the aggregates I noticed were missing).
Would you be fine with that?

> For pg13 and beyond, I liked Tom's idea of installing dummy functions
> for tables without storage -- that seems safer.

Not sure about that for v13.  That would be invasive post-beta.
--
Michael

  7. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-05-26T03:00:57Z

    On Mon, May 25, 2020 at 06:29:10PM +0900, Michael Paquier wrote:
> Perhaps you are right though, and that we don't need to spend this
> much energy into improving the error messages so I am fine to discard
> this part.  At the end, in order to remove the crashes, you just need
> to keep around the two RELKIND_HAS_STORAGE() checks.  But I would
> rather keep these two to use ereport(ERRCODE_FEATURE_NOT_SUPPORTED)
> instead of elog(), and keep the test coverage of the previous patch
> (including the tests for the aggregates I noticed were missing).
> Would you be fine with that?

And this means the attached.  Thoughts are welcome.
--
Michael

  8. Re: segmentation fault using currtid and partitioned tables

    Jaime Casanova <jaime.casanova@2ndquadrant.com> — 2020-05-27T05:29:39Z

    On Mon, 25 May 2020 at 22:01, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, May 25, 2020 at 06:29:10PM +0900, Michael Paquier wrote:
> > Perhaps you are right though, and that we don't need to spend this
> > much energy into improving the error messages so I am fine to discard
> > this part.  At the end, in order to remove the crashes, you just need
> > to keep around the two RELKIND_HAS_STORAGE() checks.  But I would
> > rather keep these two to use ereport(ERRCODE_FEATURE_NOT_SUPPORTED)
> > instead of elog(), and keep the test coverage of the previous patch
> > (including the tests for the aggregates I noticed were missing).
> > Would you be fine with that?
>
> And this means the attached.  Thoughts are welcome.

so, currently the patch just installs protections on both currtid_*
functions and adds some tests... therefore we can consider it as a bug
fix and let it go in 13? actually also backpatch in 12...

patch works, server doesn't crash anymore

only point to mention is a typo (a missing "l") in an added comment:

+ *     currtid_byrename

-- 
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-05-27T06:03:46Z

    On Wed, May 27, 2020 at 12:29:39AM -0500, Jaime Casanova wrote:
> so, currently the patch just installs protections on both currtid_*
> functions and adds some tests... therefore we can consider it as a bug
> fix and let it go in 13? actually also backpatch in 12...

Yes, and it has the advantage to be simple.

> only point to mention is a typo (a missing "l") in an added comment:
> 
> + *     currtid_byrename

Oops, thanks.
--
Michael

  10. Re: segmentation fault using currtid and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-27T16:53:23Z

    On 2020-May-26, Michael Paquier wrote:

> On Mon, May 25, 2020 at 06:29:10PM +0900, Michael Paquier wrote:
> > Perhaps you are right though, and that we don't need to spend this
> > much energy into improving the error messages so I am fine to discard
> > this part.  At the end, in order to remove the crashes, you just need
> > to keep around the two RELKIND_HAS_STORAGE() checks.  But I would
> > rather keep these two to use ereport(ERRCODE_FEATURE_NOT_SUPPORTED)
> > instead of elog(), and keep the test coverage of the previous patch
> > (including the tests for the aggregates I noticed were missing).
> > Would you be fine with that?
> 
> And this means the attached.  Thoughts are welcome.

Yeah, this looks good to me.  I would have used elog() instead, but
I don't care enough ... as a translator, I can come up with a message as
undecipherable as the original without worrying too much, since I
suspect nobody will ever see it in practice.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: segmentation fault using currtid and partitioned tables

    Andres Freund <andres@anarazel.de> — 2020-05-29T00:55:59Z

    Hi,

On 2020-05-22 19:32:57 -0400, Alvaro Herrera wrote:
> On 2020-Apr-09, Michael Paquier wrote:
> 
> > 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
> 
> Yeah, they're there solely for ODBC as far as I know.

And there only for very old servers (< 8.2), according to Hiroshi
Inoue. Found that out post 12 freeze. I was planning to drop them for
13, but I unfortunately didn't get around to do so :(

I guess we could decide to make a freeze exception to remove them now,
although I'm not sure the reasons for doing so are strong enough.


> > 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?
> 
> I don't know, but this stuff is so unused that your patch seems
> excessive ... and I think we'd rather not backpatch something so large.
> I propose we do something less invasive in the backbranches, like just
> throw elog() errors (nothing fancy) where necessary to avoid the
> crashes.  Even for pg12 it seems that that should be sufficient.
> 
> For pg13 and beyond, I liked Tom's idea of installing dummy functions

I concur that it seems unnecessary to make these translatable, even with
the reduced scope from
https://www.postgresql.org/message-id/20200526025959.GE6155%40paquier.xyz

Greetings,

Andres Freund

  12. Re: segmentation fault using currtid and partitioned tables

    Andres Freund <andres@anarazel.de> — 2020-05-29T01:05:06Z

    Hi,

On 2020-04-05 12:51:56 -0400, Tom Lane wrote:
> (2) The proximate cause of the crash is that rd_tableam is zero,
> so that the interface functions in tableam.h just crash hard.
> This seems like a pretty bad thing; does anyone want to promise
> that there are no other oversights of the same ilk elsewhere,
> and never will be?
> 
> I think it might be a good idea to make relations-without-storage
> set up rd_tableam as a vector of dummy functions that will throw
> some suitable complaint about "relation lacks storage".  NULL is
> a horrible default for this.

I don't have particularly strong views here. I can see a benefit to such
a pseudo AM. I can even imagine that there might some cases where we
would actually introduce some tableam functions for e.g. partitioned or
views tables, to centralize their handling more, instead of having such
considerations more distributed.  Clearly not worth actively trying to
do that for all existing code dealing with such relkinds, but there
might be cases where it's worthwhile.

OTOH, it's kinda annoying having to maintain a not insignificant number
of functions that needs to be updated whenever the tableam interface
evolves.  I guess we could partially hack our way through that by having
one such function, and just assigning it to all the mandatory callbacks
by way of a void cast. But that'd be mighty ugly.

Greetings,

Andres Freund

  13. Re: segmentation fault using currtid and partitioned tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-29T01:11:50Z

    Andres Freund <andres@anarazel.de> writes:
> On 2020-04-05 12:51:56 -0400, Tom Lane wrote:
>> I think it might be a good idea to make relations-without-storage
>> set up rd_tableam as a vector of dummy functions that will throw
>> some suitable complaint about "relation lacks storage".  NULL is
>> a horrible default for this.

> OTOH, it's kinda annoying having to maintain a not insignificant number
> of functions that needs to be updated whenever the tableam interface
> evolves.

That argument sounds pretty weak.  If you're making breaking changes
in the tableam API, updating the signatures (not even any code) of
some dummy functions seems like by far the easiest part.

			regards, tom lane

  14. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-05-29T06:48:40Z

    On Thu, May 28, 2020 at 05:55:59PM -0700, Andres Freund wrote:
> And there only for very old servers (< 8.2), according to Hiroshi
> Inoue. Found that out post 12 freeze. I was planning to drop them for
> 13, but I unfortunately didn't get around to do so :(

[... digging ...]
Ah, I think I see your point from the code.  That's related to the use
of RETURNING for ctids.

> I guess we could decide to make a freeze exception to remove them now,
> although I'm not sure the reasons for doing so are strong enough.

Not sure that's a good thing to do after beta1 for 13, but there is an
argument for that in 14.  FWIW, my company is a huge user of the ODBC
driver (perhaps the biggest one?), and we have nothing even close to
8.2.

> I concur that it seems unnecessary to make these translatable, even with
> the reduced scope from
> https://www.postgresql.org/message-id/20200526025959.GE6155%40paquier.xyz

Okay, I have switched the patch to do that.  Any comments or
objections?
--
Michael

  15. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-06-01T01:57:29Z

    On Fri, May 29, 2020 at 03:48:40PM +0900, Michael Paquier wrote:
> Okay, I have switched the patch to do that.  Any comments or
> objections?

Applied this one then.  I also got to check the ODBC driver in more
details, and I am indeed not seeing those functions getting used.
One extra thing to know is that the ODBC driver requires libpq from at
least 9.2, which may give one more argument to just remove them.

NB: prion has been failing, just looking into it.
--
Michael

  16. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-06-01T02:20:55Z

    On Mon, Jun 01, 2020 at 10:57:29AM +0900, Michael Paquier wrote:
> Applied this one then.  I also got to check the ODBC driver in more
> details, and I am indeed not seeing those functions getting used.
> One extra thing to know is that the ODBC driver requires libpq from at
> least 9.2, which may give one more argument to just remove them.
> 
> NB: prion has been failing, just looking into it.

Woah.  This one is old, good catch from -DRELCACHE_FORCE_RELEASE.  It
happens that since its introduction in a3519a2 from 2002,
currtid_for_view() in tid.c closes the view and then looks at a RTE
from it.  I have reproduced the issue and the patch attached takes
care of the problem.  Would it be better to backpatch all the way down
or is that not worth caring about?
--
Michael

  17. Re: segmentation fault using currtid and partitioned tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-01T02:26:54Z

    Michael Paquier <michael@paquier.xyz> writes:
> Woah.  This one is old, good catch from -DRELCACHE_FORCE_RELEASE.  It
> happens that since its introduction in a3519a2 from 2002,
> currtid_for_view() in tid.c closes the view and then looks at a RTE
> from it.  I have reproduced the issue and the patch attached takes
> care of the problem.  Would it be better to backpatch all the way down
> or is that not worth caring about?

Ugh.  Aside from the stale-pointer-deref problem, once we drop the lock
we can't even be sure the table still exists.  +1 for back-patch.

			regards, tom lane

  18. Re: segmentation fault using currtid and partitioned tables

    Michael Paquier <michael@paquier.xyz> — 2020-06-01T05:55:27Z

    On Sun, May 31, 2020 at 10:26:54PM -0400, Tom Lane wrote:
> Ugh.  Aside from the stale-pointer-deref problem, once we drop the lock
> we can't even be sure the table still exists.  +1 for back-patch.

Thanks.  Fixed down to 9.5 then to make prion happier.
--
Michael