Date: Mon, 16 Aug 2021 20:09:00 -0400
From: "alvherre@alvh.no-ip.org" <alvherre@alvh.no-ip.org>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	"x4mmm@yandex-team.ru" <x4mmm@yandex-team.ru>,
	"a.lubennikova@postgrespro.ru" <a.lubennikova@postgrespro.ru>,
	"hlinnaka@iki.fi" <hlinnaka@iki.fi>,
	"matsumura.ryo@fujitsu.com" <matsumura.ryo@fujitsu.com>,
	"masao.fujii@gmail.com" <masao.fujii@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: archive status ".ready" files may be created too early
Message-ID: <202108170009.urejfpnljykf@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <867B852E-E892-4DD7-8E29-CE74FDBBA234@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202108170009.urejfpnljykf%40alvherre.pgsql>
Precedence: bulk

So why do we call this structure "record boundary map", when the
boundaries it refers to are segment boundaries?  I think we should call
it "segment boundary map" instead ... and also I think we should use
that name in the lock that protects it, so instead of ArchNotifyLock, it
could be SegmentBoundaryLock or perhaps WalSegmentBoundaryLock.

The reason for the latter is that I suspect the segment boundary map
will also have a use to fix the streaming replication issue I mentioned
earlier in the thread.  This also makes me think that we'll want the wal
record *start* address to be in the hash table too, not just its *end*
address.  So we'll use the start-1 as position to send, rather than the
end-of-segment when GetFlushRecPtr() returns that.

As for 0xFFFFFFFFFFFFFFFF, I think it would be cleaner to do a
#define MaxXLogSegNo with that value in the line immediately after
typedef XLogSegNo, rather than use the numerical value directly in the
assignment.

Typo in comment atop RemoveRecordBoundariesUpTo: it reads "up to an",
should read "up to and".

I think the API of GetLatestRecordBoundarySegment would be better by
returning the boolean and having the segment as out argument.  Then you
could do the caller more cleanly,

if (GetLatestRecordBoundarySegment(last_notified, flushed, &latest_boundary_segment))
{
   SetLastNotified( ... );
   RemoveRecordBoundaries( ... );
   LWLockRelease( ... );
   for (..)
     XLogArchiveNotifySeg(...);
   PgArchWakeup();
}
else
   LWLockRelease(...);

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)