Re: pgsql: Fix and document lock handling for in-memory replication slot da
On Sun, Jun 10, 2018 at 10:45:04AM +, Michael Paquier wrote: > Fix and document lock handling for in-memory replication slot data > > [... snip ...] > > Details > --- > https://git.postgresql.org/pg/commitdiff/9e149c847f398793ec1641885434dcd10837d89d My apologies here. This commit has an outdated author timestamp, and I noticed it just after commit. I will be more careful in the future about that while cherry-picking changes across branches. I have as well upgraded by log configuration to that, which is mainly a matter of taste but it shows name and timestamp for both author and committer (%n stands for a newline, one configuration parameter should be on the same line as far as I know). [format] pretty = format:%C(blue)commit: %H%C(reset)%n %C(green)author: %aN <%aE>%C(reset)%n %C(green)date: %aD%C(reset)%n %C(yellow)committer: %cN <%ce>%C(reset)%n %C(yellow)date: %cD%C(reset)%n%B Feel free to reuse it, there are many other ways to set up that as well as git documentation says here: https://git-scm.com/docs/pretty-formats -- Michael signature.asc Description: PGP signature
Re: pgsql: Fix and document lock handling for in-memory replication slot da
Hi, On 2018-06-10 10:45:04 +, Michael Paquier wrote: > Fix and document lock handling for in-memory replication slot data > > While debugging issues on HEAD for the new slot forwarding feature of > Postgres 11, some monitoring of the code surrounding in-memory slot data > has proved that the lock handling may cause inconsistent data to be read > by read-only callers of slot functions, particularly > pg_get_replication_slots() which fetches data for the system view > pg_replication_slots, or modules looking directly at slot information. > > The code paths involved in those problems concern logical decoding > initialization (down to 9.4) and WAL reservation for slots (new as of > 10). > > A set of comments documenting all the lock handlings, particularly the > dependency with LW locks for slots and the in_use flag as well as the > internal mutex lock is added, based on a suggested by Simon Riggs. > > Some of the fixed code exists down to 9.4 where WAL decoding has been > introduced, but as those race conditions are really unlikely going to > happen as those concern code paths for slot and decoding creation, just > fix the problem on HEAD. You can't do things like: /* start at current insert position */ + SpinLockAcquire(&slot->mutex); slot->data.restart_lsn = GetXLogInsertRecPtr(); + SpinLockRelease(&slot->mutex); a) we don't call external functions with a spinlock held. As a rule. It's too hard to se what happens in that other function / too likely to change independently. b) we most certainly don't do it if the other function also acquires a spinlock: XLogRecPtr GetXLogInsertRecPtr(void) { XLogCtlInsert *Insert = &XLogCtl->Insert; uint64 current_bytepos; SpinLockAcquire(&Insert->insertpos_lck); current_bytepos = Insert->CurrBytePos; SpinLockRelease(&Insert->insertpos_lck); return XLogBytePosToRecPtr(current_bytepos); } Nesting spinlock means that you'd need to be very careful about whether all lockers use the same order. And be ok with the system stalled until the PANIC if it failed. Same is true for the codepaths calling GetRedoRecPtr(). I don't object to the general idea of adding locking - although the benefit are nearly guaranteed to be cosmetic - but this has the potential to make things worse. - Andres
Re: pgsql: Fix and document lock handling for in-memory replication slot da
On Mon, Jun 11, 2018 at 09:49:52AM -0700, Andres Freund wrote: > Same is true for the codepaths calling GetRedoRecPtr(). You are right. I'll fix in a minute. A first commit's stress make things really harder to get right... > I don't object to the general idea of adding locking - although the > benefit are nearly guaranteed to be cosmetic - but this has the > potential to make things worse. Thanks. -- Michael signature.asc Description: PGP signature
Re: pgsql: Fix and document lock handling for in-memory replication slot da
On Sun, Jun 10, 2018 at 10:45:04AM +, Michael Paquier wrote: > Fix and document lock handling for in-memory replication slot data > > [... snip ...] > > Details > --- > https://git.postgresql.org/pg/commitdiff/9e149c847f398793ec1641885434dcd10837d89d My apologies here. This commit has an outdated author timestamp, and I noticed it just after commit. I will be more careful in the future about that while cherry-picking changes across branches. I have as well upgraded by log configuration to that, which is mainly a matter of taste but it shows name and timestamp for both author and committer (%n stands for a newline, one configuration parameter should be on the same line as far as I know). [format] pretty = format:%C(blue)commit: %H%C(reset)%n %C(green)author: %aN <%aE>%C(reset)%n %C(green)date: %aD%C(reset)%n %C(yellow)committer: %cN <%ce>%C(reset)%n %C(yellow)date: %cD%C(reset)%n%B Feel free to reuse it, there are many other ways to set up that as well as git documentation says here: https://git-scm.com/docs/pretty-formats -- Michael signature.asc Description: PGP signature
Re: pgsql: Fix and document lock handling for in-memory replication slot da
Hi, On 2018-06-10 10:45:04 +, Michael Paquier wrote: > Fix and document lock handling for in-memory replication slot data > > While debugging issues on HEAD for the new slot forwarding feature of > Postgres 11, some monitoring of the code surrounding in-memory slot data > has proved that the lock handling may cause inconsistent data to be read > by read-only callers of slot functions, particularly > pg_get_replication_slots() which fetches data for the system view > pg_replication_slots, or modules looking directly at slot information. > > The code paths involved in those problems concern logical decoding > initialization (down to 9.4) and WAL reservation for slots (new as of > 10). > > A set of comments documenting all the lock handlings, particularly the > dependency with LW locks for slots and the in_use flag as well as the > internal mutex lock is added, based on a suggested by Simon Riggs. > > Some of the fixed code exists down to 9.4 where WAL decoding has been > introduced, but as those race conditions are really unlikely going to > happen as those concern code paths for slot and decoding creation, just > fix the problem on HEAD. You can't do things like: /* start at current insert position */ + SpinLockAcquire(&slot->mutex); slot->data.restart_lsn = GetXLogInsertRecPtr(); + SpinLockRelease(&slot->mutex); a) we don't call external functions with a spinlock held. As a rule. It's too hard to se what happens in that other function / too likely to change independently. b) we most certainly don't do it if the other function also acquires a spinlock: XLogRecPtr GetXLogInsertRecPtr(void) { XLogCtlInsert *Insert = &XLogCtl->Insert; uint64 current_bytepos; SpinLockAcquire(&Insert->insertpos_lck); current_bytepos = Insert->CurrBytePos; SpinLockRelease(&Insert->insertpos_lck); return XLogBytePosToRecPtr(current_bytepos); } Nesting spinlock means that you'd need to be very careful about whether all lockers use the same order. And be ok with the system stalled until the PANIC if it failed. Same is true for the codepaths calling GetRedoRecPtr(). I don't object to the general idea of adding locking - although the benefit are nearly guaranteed to be cosmetic - but this has the potential to make things worse. - Andres
Re: pgsql: Fix and document lock handling for in-memory replication slot da
On Mon, Jun 11, 2018 at 09:49:52AM -0700, Andres Freund wrote: > Same is true for the codepaths calling GetRedoRecPtr(). You are right. I'll fix in a minute. A first commit's stress make things really harder to get right... > I don't object to the general idea of adding locking - although the > benefit are nearly guaranteed to be cosmetic - but this has the > potential to make things worse. Thanks. -- Michael signature.asc Description: PGP signature