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. So, I am okay with the current code but want to see if anyone else thinks otherwise or if I am missing something. -- With Regards, Amit Kapila.
