Обсуждение: [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(-)
Re: [COMMITTERS] pgsql: contrib/amcheck needs RecentGlobalXmin to bePGDLLIMPORT'ified.
От
Andres Freund
Дата:
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
Re: [COMMITTERS] pgsql: contrib/amcheck needs RecentGlobalXmin to bePGDLLIMPORT'ified.
От
Andres Freund
Дата:
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
Re: [COMMITTERS] pgsql: contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.
От
Peter Geoghegan
Дата:
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