Em sex., 7 de mar. de 2025 às 15:54, Álvaro Herrera <alvhe...@alvh.no-ip.org> escreveu:
> Hello > > I find that this is still quite broken -- both the original, and your > patch. I have already complained about a fundamental bug in the > original in [1]. In addition to what I reported there, in the unpatched > code I noticed that we're wasting memory and CPU by comparing the > qualified table name, when it would be faster to just store the table > OID and do the comparison based on that. Because the REINDEX INDEX > query only needs to specify the *index* name, not the table name, we > don't need the table name to be stored in the indices_tables_list: we > can convert it into an OID list and store that instead. The strcmp > becomes a numeric comparison. Yay. This is of course a pretty trivial, > almost meaningless change. > > [1] https://postgr.es/m/202503071820.j25zn3lo4hvn@alvherre.pgsql > > But your patch also makes the mistake that indices_table_list is passed > as a pointer by value, so the list that is filled by > get_parallel_tabidx_list() can never have any effect. I wonder if you > tested this patch at all. With your patch and the sample setup I posted > in [1], the program doesn't do anything. It never calls REINDEX at all. > It is a dead program. It's so bogus that in fact, there's no program, > it's just a bug that takes a lot of disk space. > > For this to work, we need to pass that list (the output list) as a > pointer reference, so that the caller can _receive_ the output list. > > We also get this compiler warning: > > /pgsql/source/master/src/bin/scripts/reindexdb.c: In function > ‘get_parallel_tabidx_list’: > /pgsql/source/master/src/bin/scripts/reindexdb.c:782:17: warning: this > ‘if’ clause does not guard... [-Wmisleading-indentation] > 782 | if (cell != index_list->head) > | ^~ > /pgsql/source/master/src/bin/scripts/reindexdb.c:785:25: note: ...this > statement, but the latter is misleadingly indented as if it were guarded by > the ‘if’ > 785 | appendQualifiedRelation(&catalog_query, > cell->val, conn, echo); > | ^~~~~~~~~~~~~~~~~~~~~~~ > > Not to mention the 'git apply' warnings: > > I schmee: master 0$ git apply > /tmp/v3-simplifies-reindex-one-database-reindexdb.patch > /tmp/v3-simplifies-reindex-one-database-reindexdb.patch:191: space before > tab in indent. > > fmtQualifiedIdEnc(PQgetvalue(res, i, 1), > /tmp/v3-simplifies-reindex-one-database-reindexdb.patch:192: space before > tab in indent. > > PQgetvalue(res, i, 0), > /tmp/v3-simplifies-reindex-one-database-reindexdb.patch:193: space before > tab in indent. > > PQclientEncoding(conn))); > advertencia: 3 líneas agregan errores de espacios en blanco. > > > Anyway, my version of this is attached. It fixes the problems with your > patch, but not Orlov's fundamental bug. Without fixing that bug, this > program does not deserve this supposedly parallel mode that doesn't > actually deliver. I wonder if we shouldn't just revert 47f99a407de2 > completely. > > You, on the other hand, really need to be much more careful with the > patches you submit. > Yes of course, thank you for making the necessary corrections. best regards, Ranier Vilela
