Thread: pgsql: Move I/O before the index_update_stats() buffer lock region.
Move I/O before the index_update_stats() buffer lock region. Commit a07e03fd8fa7daf4d1356f7cb501ffe784ea6257 enlarged the work done here under the pg_class heap buffer lock. Two preexisting actions are best done before holding that lock. Both RelationGetNumberOfBlocks() and visibilitymap_count() do I/O, and the latter might exclusive-lock a visibility map buffer. Moving these reduces contention and risk of undetected LWLock deadlock. Back-patch to v12, like that commit. Discussion: https://postgr.es/m/20241031200139.b4@rfd.leadboat.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b412f402d1e020c5dac94f3bf4a005db69519b99 Modified Files -------------- src/backend/catalog/index.c | 62 +++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 22 deletions(-)
Noah Misch <noah@leadboat.com> writes:
> Move I/O before the index_update_stats() buffer lock region.
In branches before 17, this commit is causing some compilers
to complain:
index.c: In function 'index_update_stats':
index.c:2913:24: warning: 'relpages' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (rd_rel->relpages != (int32) relpages)
~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
I'm seeing that with RHEL8's gcc 8.5.0, and trawling the buildfarm
shows at least 40 animals similarly unhappy. (I noticed this because
mamba, which is using -Werror, is actually failing.)
regards, tom lane
On Sat, Nov 02, 2024 at 10:32:56PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > Move I/O before the index_update_stats() buffer lock region. > > In branches before 17, this commit is causing some compilers > to complain: > > index.c: In function 'index_update_stats': > index.c:2913:24: warning: 'relpages' may be used uninitialized in this function [-Wmaybe-uninitialized] > if (rd_rel->relpages != (int32) relpages) > ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ > > I'm seeing that with RHEL8's gcc 8.5.0, and trawling the buildfarm > shows at least 40 animals similarly unhappy. (I noticed this because > mamba, which is using -Werror, is actually failing.) Hopefully now the code won't fool any compiler.
Noah Misch <noah@leadboat.com> writes:
> On Sat, Nov 02, 2024 at 10:32:56PM -0400, Tom Lane wrote:
>> In branches before 17, this commit is causing some compilers
>> to complain:
> Hopefully now the code won't fool any compiler.
I saw your commit, and I'm content with it, but did you figure
out why the warning only appears pre-17? (At least, that's
what I got locally.)
regards, tom lane
On Sat, Nov 02, 2024 at 11:13:57PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Sat, Nov 02, 2024 at 10:32:56PM -0400, Tom Lane wrote:
> >> In branches before 17, this commit is causing some compilers
> >> to complain:
>
> > Hopefully now the code won't fool any compiler.
>
> I saw your commit, and I'm content with it, but did you figure
> out why the warning only appears pre-17? (At least, that's
> what I got locally.)
Nope. The principles behind the variable's lifecycle didn't change. Here's
the function's diff between v16 and v17. (Same diff all the way from v12 to
v17.) Perhaps the reference to non-local variable IsBinaryUpgrade changed the
compiler's deductive capability, somehow.
@@ -2836,7 +2806,11 @@ index_update_stats(Relation rel,
if (reltuples == 0 && rel->rd_rel->reltuples < 0)
reltuples = -1;
- update_stats = reltuples >= 0;
+ /*
+ * Don't update statistics during binary upgrade, because the indexes are
+ * created before the data is moved into place.
+ */
+ update_stats = reltuples >= 0 && !IsBinaryUpgrade;
/*
* Finish I/O and visibility map buffer locks before