Re: Rework the way multixact truncations work

On Thu, Jul 2, 2015 at 2:28 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-07-02 13:58:45 -0400, Robert Haas wrote:
>> On Thu, Jul 2, 2015 at 11:52 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > Will look at 0003 next.
>>
>> + appendStringInfo(buf, "offsets [%u, %u), members [%u, %u)",
>>
>> I don't think we typically use this style for notating intervals.
>
> I don't think we really have a very consistent style for xlog messages -
> this seems to describe the meaning accurately?

Although I realize this is supposed to be interval notation, I'm not
sure everyone will immediately figure that out. I believe it has
created some confusion in the past. I'm not going to spend a lot of
time arguing with you about it, but I'd do something else, like
offsets from %u stop before %u, members %u stop before %u.

>> [several good points]
>
>> + (errmsg("performing legacy multixact truncation,
>> upgrade master")));
>>
>> This message needs work. I'm not sure exactly what it should say, but
>> I'm pretty sure that's not clear enough.
>>
>> I seriously, seriously doubt that it is a good idea to perform the
>> legacy truncation from MultiXactAdvanceOldest() rather than
>> TruncateMultiXact().
>
> But where should TruncateMultiXact() be called from? I mean, we could
> move the logic from inside MultiXactAdvanceOldest() to some special case
> in the replay routine, but what'd be the advantage?

I think you should call it from where TruncateMultiXact() is being
called from today. Doing legacy truncations from a different place
than we're currently doing them just gives us more ways to be wrong.

>> The checkpoint hasn't really happened at that point yet; you might
>> truncate away stuff, then crash before the checkpoint is complete, and
>> then we you restart recovery, you've got trouble.
>
> We're only talking about restartpoints here, right? And I don't see the
> problem - we don't read the slru anymore until the end of recovery, and
> the end of recovery can't happen before reaching the minimum revovery
> location?

You're still going to have to read the SLRU for as long as you are
doing legacy truncations, at least.

>> If TruncateMultiXact() fails to acquire MultiXactTruncationLock right
>> away, does it need to wait, or could it ConditionalAcquire and bail
>> out if the lock isn't obtained?
>
> That seems like premature optimization to me. And one that's not that
> easy to do correctly - what if the current caller actually has a new,
> lower, minimum mxid?

Doesn't the next truncation just catch up? But sure, I agree this is
inessential (and maybe better left alone for now).

>> I'm not convinced that it's a good idea to remove
>> lastCheckpointedOldest and replace it with nothing. It seems like a
>> very good idea to have two separate pieces of state in shared memory:
>
>> - The oldest offset that we think anyone might need to access to make
>> a visibility check for a tuple.
>> - The oldest offset that we still have on disk.
>>
>> The latter would now need to be called something else, not
>> lastCheckpointedOldest, but I think it's still good to have it.
>
>> Otherwise, I don't see how you protect against the on-disk state
>> wrapping around before you finish truncating, and then maybe
>> truncation eats something that was busy getting reused.
>
> Unless I miss something the stop limits will prevent that from
> happening? SetMultiXactIdLimit() is called only *after* the truncation
> has finished?

Hmm, that might be, I'd have to reread the patch. The reason we
originally had it this way was because VACUUM was updating the limit
and then checkpoint was truncating, but now I guess vacuum + truncate
happen so close together that you might only need one value.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company