pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed

From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masahiko.sawada@2ndquadrant.com
Cc: masao.fujii@oss.nttdata.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
Date: Thu, 16 Apr 2020 11:39:06 -0400
Message-ID: <23215.1587051546@sss.pgh.pa.us> (raw)
In-Reply-To: <20200416.182626.526651137261602571.horikyota.ntt@gmail.com>
References: <22258.1586964709@sss.pgh.pa.us>
	<20200416.162241.2092362802599118592.horikyota.ntt@gmail.com>
	<CA+fd4k7_wcpZSwK0Axq5ag1Hhb7cMgp2b_CbdZegRTSzZtQAFA@mail.gmail.com>
	<20200416.182626.526651137261602571.horikyota.ntt@gmail.com>

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> [ syncrep-fixes-4.patch ]

I agree that we could probably improve the clarity of this code with
further rewriting, but I'm still very opposed to the idea of having
callers know that the first num_sync array elements are the active
ones.  It's wrong (or at least different from current behavior) for
quorum mode, where there might be more than num_sync walsenders to
consider.  And it might not generalize very well to other syncrep
selection rules we might add in future, which might also not have
exactly num_sync interesting walsenders.  So I much prefer an API
definition that uses bool flags in an array that has no particular
ordering (so far as the callers know, anyway).  If you don't like
is_sync_standby, how about some more-neutral name like is_active
or is_interesting or include_position?

I dislike the proposed comment revisions in SyncRepReleaseWaiters,
too, particularly the change to say that what we're "announcing"
is the ability to release waiters.  You did not change the actual
log messages, and you would have gotten a lot of pushback if
you tried, because the current messages make sense to users and
something like that would not.  But by the same token this new
comment isn't too helpful to somebody reading the code.

(Actually, I wonder why we even have the restriction that only
sync standbys can release waiters.  It's not like they are
going to get different results from SyncRepGetSyncRecPtr than
any other walsender would.  Maybe we should just drop all the
am_sync logic?)

			regards, tom lane

view thread (38+ messages)  latest in thread

Message-ID: <23215.1587051546@sss.pgh.pa.us>
Permalink:  ../23215.1587051546@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/23215.1587051546@sss.pgh.pa.us

copy link · copy postgr.es

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, horikyota.ntt@gmail.com, masahiko.sawada@2ndquadrant.com, masao.fujii@oss.nttdata.com, pgsql-hackers@lists.postgresql.org
  Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
  In-Reply-To: <23215.1587051546@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox