Since time is short and it seems better to get the xlog page magic
bump done before beta2, I've gone ahead and committed this, but there
are two points here that seem to warrant some input from other senior
hackers. Andres and I have discussed these points off list but
without reaching a meeting of the minds.
On Fri, Jun 17, 2016 at 2:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Having an XLogInsert() in contrib makes me more than a bit squeamish. I
>> think it'd be fair bit better to have that section of code in
>> visibilitymap.c, and then call that from the extension.
>
> I thought about that too, but it seemed like it was just bloating the
> core server for no real reason. It's not like contrib is off in space
> someplace.
So, Andres thinks it's a bad idea to have an XLogInsert() call in
contrib, and I don't think it matters. Anyone else have an opinion?
Andres, do you want to explain the nature of your concern further?
>>> + /* Don't keep the relation locked any longer than necessary! */
>>> + relation_close(rel, AccessExclusiveLock);
>>
>> Don't think that's a good idea. We use transactional semantics for a
>> good reason, and the function returns afterwards anyway.
>
> Yeah, but if you want to blow away a bunch of visibility map forks at
> one go, you're not going to appreciate this function collecting all of
> those locks at the same time. This is also why VACUUM starts a
> separate transaction for each table.
Andres has a concern here that there might be a problem related to
invalidation messages. Currently, the only invalidation message
that's getting generated here is a non-transactional smgr
invalidation, which is sent before the lock is released and therefore
no race exists. However, Andres is concerned that this may at a
minimum not be very future-proof and possibly unsafe even today. To
me, however, this doesn't seem particularly different than what e.g.
lazy_truncate_heap() always does and has done for a long time. More
input here is welcome and appreciated - it would be good not to screw
this up.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company