Date: Sun, 20 Mar 2022 14:40:50 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Dipesh Pandit <dipesh.pandit@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Jeevan Ladhe <jeevanladhe.os@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"Shinoda, Noriyoshi (PN Japan FSIP)" <noriyoshi.shinoda@hpe.com>,
	Abhijit Menon-Sen <ams@toroid.org>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>,
	Mark Dilger <mark.dilger@enterprisedb.com>,
	pgsql-hackers@postgresql.org,
	tushar <tushar.ahuja@enterprisedb.com>
Subject: Re: refactoring basebackup.c (zstd workers)
Message-ID: <20220320194050.GX28503@telsasoft.com>
References: <20220312015253.zokhkwdn5xbbd4vf@alap3.anarazel.de>
 <CA+TgmoZ10QPYaUxe8+RfJT5ptyDe-ML=oE_FyyFa+sNS2GBgZw@mail.gmail.com>
 <CAN1g5_F0KBgitEVLO8T0ynaBcQBfRMCjH9wKguDJZpX1hF3gFA@mail.gmail.com>
 <20220314163547.GT28503@telsasoft.com>
 <CA+TgmoaYrNxoGB8x_7Fk5uKXfbyMVAdSXTnfQN-1zu_3gB_v=Q@mail.gmail.com>
 <20220314171152.GU28503@telsasoft.com>
 <CA+TgmoYfV1ASq0QVR1QHaJnXfr+fPLoPByQL1SfWqjUuKtX_UA@mail.gmail.com>
 <CA+TgmoYvpetyRAbbg1M8b3-iHsaN4nsgmWPjOENu5-doHuJ7fA@mail.gmail.com>
 <20220317194130.GM28503@telsasoft.com>
 <CA+TgmoaaDXOT+vFDcPrnXjPTU7oFPY2_pkeQFTdSGJfrqO2LNQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmoaaDXOT+vFDcPrnXjPTU7oFPY2_pkeQFTdSGJfrqO2LNQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220320194050.GX28503%40telsasoft.com>
Precedence: bulk

On Sun, Mar 20, 2022 at 03:05:28PM -0400, Robert Haas wrote:
> On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > -                                                errmsg("unrecognized compression algorithm: \"%s\"",
> > +                                                errmsg("unrecognized compression algorithm \"%s\"",
> >
> > Most other places seem to say "compression method".  So I'd suggest to change
> > that here, and in doc/src/sgml/ref/pg_basebackup.sgml.
> 
> I'm not sure that's really better, and I don't think this patch is
> introducing an altogether novel usage. I think I would probably try to
> standardize on algorithm rather than method if I were standardizing
> the whole source tree, but I think we can leave that discussion for
> another time.

The user-facing docs are already standardized using "compression method", with
2 exceptions, of which one is contrib/ and the other is what I'm suggesting to
make consistent here.

$ git grep 'compression algorithm' doc
doc/src/sgml/pgcrypto.sgml:    Which compression algorithm to use.  Only available if
doc/src/sgml/ref/pg_basebackup.sgml:        compression algorithm is selected, or if server-side compression

> > +                       result->parse_error =
> > +                               pstrdup("found empty string where a compression option was expected");
> >
> > Needs to be localized with _() ?
> > Also, document that it's pstrdup'd.
> 
> Did the latter. The former would need to be fixed in a bunch of places
> and while I'm happy to accept an expert opinion on exactly what needs
> to be done here, I don't want to try to do it and do it wrong. Better
> to let someone with good knowledge of the subject matter patch it up
> later than do a crummy job now.

I believe it just needs _("foo")
See git grep '= _('

I mentioned another issue off-list:
pg_basebackup.c:2741:10: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
 2741 |   Assert(compressloc = COMPRESS_LOCATION_SERVER);
      |          ^~~~~~~~~~~
pg_basebackup.c:2741:3: note: in expansion of macro ‘Assert’
 2741 |   Assert(compressloc = COMPRESS_LOCATION_SERVER);

This crashes the server using your v2 patch:

src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --no-manifest --compress=server-zstd:level, |wc -c

I wonder whether the syntax should really use both ":" and ",".
Maybe ":" isn't needed at all.

This patch also needs to update the other user-facing docs.

typo: contain a an

-- 
Justin