Обсуждение: [COMMITTERS] pgsql: contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

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.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/56018bf26eec1a0b4bf20303c98065a8eb1b0c5d

Modified Files
--------------
src/include/utils/snapmgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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

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