Re: ArchiveEntry optional arguments refactoring

Alvaro Herrera <alvherre@2ndquadrant.com>

From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Chapman Flack <chap@anastigmatix.net>, Amit Khandekar <amitdkhan.pg@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2019-02-01T11:33:49Z
Lists: pgsql-hackers

Attachments

pgindent didn't like your layout with two-space indents for the struct
members :-(  I thought it was nice, but oh well.  This means we can do
away with the newline at each callsite, and I didn't like the trailing
comma (and I have vague recollections that some old compilers might
complain about them too, though maybe we retired them already.)

> * Use NULL as a default value where it was an empty string before (this
>   required few minor changes for some part of the code outside ArchiveEntry)

Ah, so this is why you changed replace_line_endings.  So the comment on
that function now is wrong -- it fails to indicate that it returns a
malloc'ed "" on NULL input.  But about half the callers want to have a
malloc'ed "-" on NULL input ... I think it'd make the code a little bit
simpler if we did that in replace_line_endings itself, maybe add a
"want_dash" bool argument, so this code

		if (!ropt->noOwner)
			sanitized_owner = replace_line_endings(te->owner);
		else
			sanitized_owner = pg_strdup("-");

can become
		sanitized_owner = replace_line_endings(te->owner, true);

I don't quite understand why the comments about line sanitization were
added in the callsites rather than in replace_line_endings itself.  I
would rename the function to sanitize_line() and put those comments
there (removing them from the callsites), then the new argument I
suggest would not be completely out of place.

What do you think?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Commits

  1. Add ArchiveOpts to pass options to ArchiveEntry

  2. Remove WITH OIDS support, change oid catalog column visibility.