Thanks a lot again for the review and comments.
On 2020-11-25 9:36 p.m., Michael Paquier wrote:
> On Wed, Nov 25, 2020 at 12:13:55PM -0800, David Zhang wrote:
>> The previous patch was based on branch "REL_13_STABLE". Now, the attached
>> new patch v2 is based on master branch. I followed the new code structure
>> using appendPQExpBuffer to append the new clause "using TABLEAM" in a proper
>> position and tested. In the meantime, I also updated the pgbench.sqml file
>> to reflect the changes.
> + <varlistentry>
> + <term><option>--table-am=<replaceable>TABLEAM</replaceable></option></term>
> + <listitem>
> + <para>
> + Create all tables with specified table access method
> + <replaceable>TABLEAM</replaceable>.
> + If unspecified, default is <literal>heap</literal>.
> + </para>
> + </listitem>
> + </varlistentry>
> This needs to be in alphabetical order.
The order issue is fixed in v3 patch.
> And what you say here is
> wrong. The default would not be heap if default_table_access_method
> is set to something else.
Right, if user change the default settings in GUC, then the default is
not `heap` any more.
> I would suggest to use table_access_method
> instead of TABLEAM,
All other options if values are required, the words are all capitalized,
such as TABLESPACE. Therefore, I used TABLEACCESSMETHOD in stead of
table_access_method in v3 patch.
> and keep the paragraph minimal, say:
> "Create tables using the specified table access method, rather than
> the default."
Updated in v3 patch.
>
> --table-am is really the best option name? --table-access-method
> sounds a bit more consistent to me.
Updated in v3 patch.
>
> + if (tableam != NULL)
> + {
> + char *escape_tableam;
> +
> + escape_tableam = PQescapeIdentifier(con, tableam, strlen(tableam));
> + appendPQExpBuffer(&query, " using %s", escape_tableam);
> + PQfreemem(escape_tableam);
> + }
Thanks for pointing this out. The order I believe is fixed in v3 patch.
> The order is wrong here, generating an unsupported grammar, see by
> yourself with this command:
> pgbench --partition-method=hash --table-am=heap -i --partitions 2
Tested in v3 patch, with a command like,
pgbench --partition-method=hash --table-access-method=heap -i --partitions 2
>
> This makes the patch trickier than it looks as the USING clause is
> located between PARTITION BY and WITH. Also, partitioned tables
> cannot use the USING clause so you need to be careful that
> createPartitions() also uses the correct table AM.
>
> This stuff needs tests.
3 test cases has been added the tap test, but honestly I am not sure if
I am following the rules. Any comments will be great.
> --
> Michael
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
