On Wed, Aug 16, 2017 at 4:55 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>
> Hello Masahiko-san,
>
>> Yeah, once custom initialization patch get committed we can extend it.
>>
>> Attached updated patch. I've incorporated the all comments from Fabien
>> to it, and changed it to single letter version.
>
>
> Patch applies and works.
>
> A few comments and questions about the code and documentation:
Thank you for the comments!
>
> Why not allow -I as a short option for --custom-initialize?
Other options for similar purpose such as --foreign-keys also have
only a long option. Since I think --custom-initialize option is in the
same context as other options I didn't add short option to it for now.
Because the options name is found by a prefix searching we can use a
short name --cu for now.
> I do not think that the --custom-initialize option needs to appear as a
> specific synopsis in the documentation, as it is now a sub-option of -i.
>
> checkCustomCmds: I would suggest to simplify test code with strchr
> and to merge the two fprintf into one, something like:
>
> if (strchr("tdpfv", *cmd) == NULL) {
> fprintf(stderr,
> "....\n"
> "....\n",
> ...);
> ...
>
> Moreover there is already an error message later if checkCustomCmds fails, I
> think
> it could be expanded and the two-line one in the function dropped entirely?
> It seems strange to have two level error messages for that.
>
> Help message looks strange. I suggest something regexp-like:
>
> --custom-initialize=[tdvpf]+
>
> I would suggest to put the various init* functions in a more logical order:
> first create table, then load data, etc.
>
> In case 0: do not exchange unlogged_tables & foreign_keys gratuiously.
>
> After checking the initial code, I understand that the previous default was
> "tdvp", but you put "tdpv". I'm unsure whether vaccuum would do something to
> the indexes and that would make more sense. In doubt, I suggest to keep the
> previous default.
>
> Maybe --foreign-keys should really do "tdvpf"?
>
> I may be okay with disallowing --foreign-keys and --no-vacuum if
> --custom-init is used,
> but then there is no need to test it again in init... I think that in any
> case 'f' and 'v'
> should always trigger the corresponding initializations.
>
> On the other hand, I think that it could be more pragmatic with these
> options, i.e. --foreign-keys could just append 'f' to the current command if
> not already there, and '--no-vacuum' could remove 'v' if there, on the fly,
> so that nothing would be banned. This would require to always have a
> malloced custom_init string. It would allow to remove the "foreign_keys"
> global variable. "is_no_vacuum" is probably still needed for benchmarking.
> This way there would be no constraints and "is_custom_init" could be dropped
> as well.
I'm inclined to remove the restriction so that we can specify
--foreign-keys, --no-vacuum and --custom-initialize at the same time.
I think a list of char would be better here rather than a single
malloced string to remove particular initialization step easily.
Thought?
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
