On Mon, Oct 21, 2024 at 5:09 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Oct 7, 2024 at 4:03 PM vignesh C <vignes...@gmail.com> wrote: > > > > On Mon, 7 Oct 2024 at 12:26, Hayato Kuroda (Fujitsu) > > <kuroda.hay...@fujitsu.com> wrote: > > > > > One comment for your patch; > > > Shouldn't we add a streaming=off case for pg_dump test? You added lines > > > but no one > > > passes it. > > > > > > > Update existing pg_dump tests to cover the streaming options, the > > attached patch has the changes for the same. > > > > @@ -5235,6 +5235,8 @@ dumpSubscription(Archive *fout, const > SubscriptionInfo *subinfo) > appendPQExpBufferStr(query, ", streaming = on"); > else if (strcmp(subinfo->substream, "p") == 0) > appendPQExpBufferStr(query, ", streaming = parallel"); > + else > + appendPQExpBufferStr(query, ", streaming = off"); > > For newer versions (>=18), we shouldn't need to specify "streaming = > parallel" while creating a subscription as that will be the default. > However, with the above code pg_dump statements will still have that. > There is nothing wrong with that but ideally, it won't be required. > Now, OTOH, doing that would require some version-handling code, which > is nothing new for pg_dump but not sure it makes sense for this > particular case. Another related point is that normally we don't > recommend people to use dump generated with pg_dump to use with lower > server versions than pg_dump itself, but the current proposed patch > will allow that. However, if we change it as I am saying that won't be > possible.
Leaving the patch as-is seems better to me. PROS - The simple code explicitly setting all parameter values is easy to understand as-is. - AFAICT it works for all that the pg_dump docs [1] guarantees. - No version handling code will be needed... - Therefore, no risk of accidentally introducing any versioning bugs. - Yields a more portable dump file (even though not guaranteed by pg_dump docs) CONS - A few more chars in the dump file? - What else? ====== [1] https://www.postgresql.org/docs/devel/app-pgdump.html Kind Regards, Peter Smith. Fujitsu Australia
