From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Tristan Partin <tristan(at)neon(dot)tech> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Use COPY for populating all pgbench tables |
Date: | 2023-07-21 02:14:57 |
Message-ID: | ZLnqIQuMX8BzyFXR@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jul 20, 2023 at 02:22:51PM -0500, Tristan Partin wrote:
> Thanks for your testing Michael. I went ahead and added a test to make sure
> that this behavior doesn't regress accidentally, but I am struggling to get
> the test to fail using the previous version of this patch. Do you have any
> advice? This is my first time writing a test for Postgres. I can recreate
> the issue outside of the test script, but not within it for whatever reason.
We're all here to learn, and writing TAP tests is important these days
for a lot of patches.
+# Check that the pgbench_branches and pgbench_tellers filler columns are filled
+# with NULLs
+foreach my $table ('pgbench_branches', 'pgbench_tellers') {
+ my ($ret, $out, $err) = $node->psql(
+ 'postgres',
+ "SELECT COUNT(1) FROM $table;
+ SELECT COUNT(1) FROM $table WHERE filler is NULL;",
+ extra_params => ['--no-align', '--tuples-only']);
+
+ is($ret, 0, "psql $table counts status is 0");
+ is($err, '', "psql $table counts stderr is empty");
+ if ($out =~ m/^(\d+)\n(\d+)$/g) {
+ is($1, $2, "psql $table filler column filled with NULLs");
+ } else {
+ fail("psql $table stdout m/^(\\d+)\n(\\d+)$/g");
+ }
+}
This is IMO hard to parse, and I'd rather add some checks for the
accounts and history tables as well. Let's use four simple SQL
queries like what I posted upthread (no data for history inserted
after initialization), as of the attached. I'd be tempted to apply
that first as a separate patch, because we've never add coverage for
that and we have specific expectations in the code from this filler
column for tpc-b. And this needs to cover both client-side and
server-side data generation.
Note that the indentation was a bit incorrect, so fixed while on it.
Attached is a v7, with these tests (should be a patch on its own but
I'm lazy to split this morning) and some more adjustments that I have
done while going through the patch. What do you think?
--
Michael
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Use-COPY-instead-of-INSERT-for-populating-all-tab.patch | text/x-diff | 11.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2023-07-21 03:08:47 | Re: Support worker_spi to execute the function dynamically. |
Previous Message | David Rowley | 2023-07-21 02:03:46 | Re: Avoid stack frame setup in performance critical routines using tail calls |