pgsql: contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

On 2017-03-10 03:55:50 +0000, Tom Lane wrote:

contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

Per buildfarm. Maybe some of the other xmin variables in snapmgr.h
ought to get this too, but for the moment I'm just interested in
un-breaking the buildfarm.

Heh, I just wanted to push a patch removing the assertion because it
doesn't add that much. There's imo no reason not to mark the variable
PGDLLIMPORT, so I'm good with this too.

- Andres

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Andres Freund <andres@anarazel.de> writes:

On 2017-03-10 03:55:50 +0000, Tom Lane wrote:

contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

Heh, I just wanted to push a patch removing the assertion because it
doesn't add that much. There's imo no reason not to mark the variable
PGDLLIMPORT, so I'm good with this too.

Oh, I hadn't looked closely enough to notice that the only reference
there was

Assert(TransactionIdIsValid(RecentGlobalXmin));

I agree: that is just about utterly useless. Let's revert my patch
and remove that Assert. I'm not eager to encourage people to reference
the xmin globals if we don't have to.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 2017-03-09 23:50:27 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2017-03-10 03:55:50 +0000, Tom Lane wrote:

contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

Heh, I just wanted to push a patch removing the assertion because it
doesn't add that much. There's imo no reason not to mark the variable
PGDLLIMPORT, so I'm good with this too.

Oh, I hadn't looked closely enough to notice that the only reference
there was

Assert(TransactionIdIsValid(RecentGlobalXmin));

I agree: that is just about utterly useless.

Well, it mirrors an existing Assert, that'd be hit when doing normal
index lookups. But I agree that a bug around this is exceedingly
unlikely at this point, so there's no coverage value in it.

Let's revert my patch
and remove that Assert. I'm not eager to encourage people to reference
the xmin globals if we don't have to.

We have a bunch of index access methods (nbtree, spgist) referencing
RecentGlobalXmin for, imo, reasonable reasons. So it doesn't seem
unreasonable to keep it available for extensions, given the amount of
work has gone into making indexes from extensions a usable thing.

Greetings,

Andres Freund

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Fri, Mar 10, 2017 at 11:47 AM, Andres Freund <andres@anarazel.de> wrote:

Assert(TransactionIdIsValid(RecentGlobalXmin));

I agree: that is just about utterly useless.

Well, it mirrors an existing Assert, that'd be hit when doing normal
index lookups. But I agree that a bug around this is exceedingly
unlikely at this point, so there's no coverage value in it.

I was in favor of just removing the assertion myself, given the
PGDLLIMPORT issue, but FWIW I am generally in favor of documenting
assertions like this. I suppose that this assertion is less likely to
ever break than most other assertions, but presumably no code ever
gets committed without somebody being pretty confident that any
assertions it happens to have will never fail to hold. It doesn't seem
productive to worry about whether or not any trivial assertions are
pulling their weight. They're justified as documentation. If the
assertion ever does fail, preventing someone from pushing buggy code,
then so much the better.

--
Peter Geoghegan

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers