On Thu, Aug 27, 2026 at 2:23 PM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> Thanks Noah and Andrew for the feedback. Based on review comments, I
> re-implemented these patches.
>
at [1], Noah mentioned that
"""
There should be one appendPQExpBuffer(delQry, "DROP ROLE ..."), not one for
plain format and another for non-plain formats. Having two creates excess
risk of format-specific bugs, something pg_dump.c has long avoided well. (I'm
echoing my postgr.es/m/20250708212819.09.nmisch@google.com review. I wrote,
"The strength of the archiver architecture shows in how rarely new features
need format-specific logic and how rarely format-specific bugs get reported."
That holds for the way pg_dump.c uses the archiver, but it doesn't hold for
the way pg_dumpall.c now uses the archiver.)
"""
That means we cannot use
if (archDumpFormat == archNull)
fprintf(OPF, "DROP ROLE"....)
else
ArchiveEntry(fout, ....)
All of the below is wrong.
+static void dropDBsText(PGconn *conn);
+static void dropTablespacesText(PGconn *conn);
+static void dropRolesText(PGconn *conn)
Therefore, we should unify it using ArchiveEntry, because we cannot
use fprintf for non-plain text formats.
With that, plain-text format also needs CreateArchive(). We must be
careful with fprintf(OPF): both RestoreArchive() and fprintf(OPF)
write to the same output, so their writes must be ordered correctly.
I also refactored check_for_invalid_global_names. if one database name
contained \r\n, call pg_fatal.
[1] https://www.postgresql.org/message-id/20260607000218.96.noahmisch%40microsoft.com
--
jian
https://www.enterprisedb.com/
