Re: [HACKERS] [PATCH] pageinspect function to decode infomasks

[Amit Kapila]

On Thu, Sep 5, 2019 at 2:17 PM Masahiko Sawada  wrote:
>
> Thanks. I hope the attached new patch fixes this issue.
>

*
+-- decode infomask flags as human readable flag names
+CREATE FUNCTION heap_infomask_flags(
+   infomask integer,
+   infomask2 integer,
+   decode_combined boolean DEFAULT false)
+RETURNS text[]
+AS 'MODULE_PATHNAME', 'heap_infomask_flags'

Isn't it better to name this function as tuple_infomask_flags or
something like that?  The other functions that start their name with
heap take page as input.  Also, I think the index-related functions
that start with index name take blk/page as input.

*
+   
+
+ heap_infomask_flags(infomask integer, infomask2
integer, decode_combined bool) returns text[]
+ 
+  heap_infomask_flags
+ 
+
+
+ 
+  heap_infomask_flags decodes the
+  t_infomask and
+  t_infomask2 returned by
+  heap_page_items into a human-readable
+  array of flag names. This can be used to see the tuple hint bits etc.
+ 

I think it is better to use one example for this function as we do for
other functions in this doc.

*
+ 
+  If decode_combined is set, combination flags like
+  HEAP_XMIN_FROZEN are output instead of raw
+  flags, HEAP_XMIN_COMMITTED and
+  HEAP_XMIN_INVALID. Default value is
+  false.
+ 

decode_combined should use parameter marker (like
decode_combined).  Instead of saying
"decode_combined" is set, you can use true/false terminology as that
is what we use for this parameter.  See explanation of "do_detoast"
that is used in function tuple_data_split in the same doc
(pageinspect.sgml) for reference.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: amcheck verification for GiST

[Andrey Borodin]

Alvaro, Peter, thanks for working on this!

> 7 сент. 2019 г., в 6:26, Peter Geoghegan  написал(а):
> 
> On Fri, Sep 6, 2019 at 3:22 PM Peter Geoghegan  wrote:
>> I'll take care of it, then.
> 
> Attached is v10, which has some comment and style fix-ups, including
> the stuff Alvaro mentioned. It also adds line pointer sanitization to
> match what I added to verify_nbtree.c in commit a9ce839a (we use a
> custom PageGetItemIdCareful() for GiST instead of a simple
> PageGetItemId()). I also added a new file/TU for the routines that are
> now common to both nbtree and GiST verification, which I named
> amcheck.c. (I'm not sure about that, but I don't like verify_nbtree.c
> having generic/common functions.)
Maybe we should PageGetItemIdCareful() to amcheck.c too?
I think it can be reused later by GIN entry tree and to some extent by SP-GiST.
SP-GiST uses redirect tuples, but do not store this information in line pointer.


> I have only had a few hours to polish this, which doesn't seem like
> enough, though was enough to fix the noticeable stuff.
Cool, thanks!

> My main concern now is the heavyweight lock strength needed by the new
> function. I don't feel particularly qualified to sign off on the
> concurrency aspects of the patch. Heikki's v6 used a ShareLock, like
> bt_index_parent_check(), but you went back to an AccessShareLock,
> Andrey. Why is this safe? I see that you do gist_refind_parent() in
> your v9 a little differently to Heikki in his v6, which you seemed to
> suggest made this safe in your e-mail on March 28, but I don't
> understand that at all.

Heikki's version was reading childblkno instead of parentblkno, thus never 
refinding parent tuple.
Also, I've added check that internal page did not become leaf. In this case we 
would be comparing heap tids with index tids, and could possibly find incorrect 
match.
But, in current GiST implementation, inner page should never become leaf. We do 
not delete inner pages.
I think we could even convert it into sanity check, but I'm afraid that once 
upon a time we will implement delete for internal pages and forget to remove 
this check.

Current lock mode is AccessShareLock.
Before we find some discrepancy in tuples we follow standard locking protocol 
of GiST Index Scan.

When we suspect key consistency violation, we hold lock on page with some 
tuple. Then we take pin and lock on page that was parent for current some time 
before.
For example of validity see gistfinishsplit(). Comments state "On entry, the 
caller must hold a lock on stack->buffer", line 1330 acquires 
LockBuffer(stack->parent->buffer, GIST_EXCLUSIVE);
This function is used during inserts, but we are not going to modify data and 
place row locks, thus neither ROW EXCLUSIVE, not ROW SHARE is necessary.

PFA v11. There is one small comment in GistScanItem. Also, I've moved 
PageGetItemIdCareful() to amcheck.c. I'm not sure that this refactoring is 
beneficial for patch, but it reduces code size.

Thanks!


Best regards, Andrey Borodin.


v11-0001-GiST-amcheck-from-Andrey.patch
Description: Binary data

Re: Yet another fast GiST build

[Andrey Borodin]

> 1 сент. 2019 г., в 15:53, Andrey Borodin  написал(а):
> 
> 
> 

Here's V3 of the patch set.
Changes:
1. Added some documentation of new sort support routines
2. Fixed bug with dirty pages

I did not add sort support procs to built-in boxes, circles and polys, since it 
may be not optimal index for them. However, for points Z-order is quite good as 
a default.

Tests only pass with fixes for GiST KNN from Alexander in other thread.

Thanks!

Best regards, Andrey Borodin.



v3-0002-Implement-GiST-build-using-sort-support.patch
Description: Binary data


v3-0001-Add-sort-support-for-point-gist_point_sortsupport.patch
Description: Binary data

Re: Change ereport level for QueuePartitionConstraintValidation

[Sergei Kornilov]

Hello

> Hearing no comments, I've pushed that patch, and marked the v12
> open item closed.

Thank you!

regards, Sergei

Re: Planning counters in pg_stat_statements (using pgss_store)

[Imai Yoshikazu]

Hello

On 2019/09/06 23:19, Tomas Vondra wrote:
> On Wed, Sep 04, 2019 at 07:19:47PM +0300, Sergei Kornilov wrote:
>>
>> ...
>>
>> Results:
>>
>>     test |   mode   | average_tps | degradation_perc
>> --+--+-+--
>> head_no_pgss | extended |   13816 |    1.000
>> patch_not_loaded | extended |   13755 |    0.996
>> head_track_none  | extended |   13607 |    0.985
>> patch_track_none | extended |   13560 |    0.981
>> head_track_top   | extended |   13277 |    0.961
>> patch_track_top  | extended |   13189 |    0.955
>> patch_track_planning | extended |   12983 |    0.940
>> head_no_pgss | prepared |   29101 |    1.000
>> head_track_none  | prepared |   28510 |    0.980
>> patch_track_none | prepared |   28481 |    0.979
>> patch_not_loaded | prepared |   28382 |    0.975
>> patch_track_planning | prepared |   28046 |    0.964
>> head_track_top   | prepared |   28035 |    0.963
>> patch_track_top  | prepared |   27973 |    0.961
>> head_no_pgss | simple   |   16733 |    1.000
>> patch_not_loaded | simple   |   16552 |    0.989
>> head_track_none  | simple   |   16452 |    0.983
>> patch_track_none | simple   |   16365 |    0.978
>> head_track_top   | simple   |   15867 |    0.948
>> patch_track_top  | simple   |   15820 |    0.945
>> patch_track_planning | simple   |   15739 |    0.941
>>
>> So I found slight slowdown with track_planning = off compared to HEAD. 
>> Possibly just at the level of measurement error. I think this is ok.
>> track_planning = on also has no dramatic impact. In my opinion 
>> proposed design with pgss_store call is acceptable.
>>
> 
> FWIW I've done some benchmarking on this too, with a single pgbench client
> running select-only test on a tiny database, in different modes (simple,
> extended, prepared). I've done that on two systems with different CPUs
> (spreadsheet with results attached).

Refering to Sergei's results, if a user, currently using pgss with 
tracking execute time, uses the new feature, a user will see 0~2.2% 
performance regression as below.

 >> head_track_top   | extended |   13277 |0.961
 >> patch_track_planning | extended |   12983 |0.940
 >> patch_track_planning | prepared |   28046 |0.964
 >> head_track_top   | prepared |   28035 |0.963
 >> head_track_top   | simple   |   15867 |0.948
 >> patch_track_planning | simple   |   15739 |0.941

If a user will not turn on the track_planning, a user will see 0.2-0.6% 
performance regression as below.

 >> head_track_top   | extended |   13277 |0.961
 >> patch_track_top  | extended |   13189 |0.955
 >> head_track_top   | prepared |   28035 |0.963
 >> patch_track_top  | prepared |   27973 |0.961
 >> head_track_top   | simple   |   15867 |0.948
 >> patch_track_top  | simple   |   15820 |0.945

> 
> I don't see any performance regression - there are some small variations
> in both directions (say, ~1%) but that's well within the noise. So I think
> the patch is fine in this regard.

+1


I also saw the codes and have one comment.

[0002 patch]
In pgss_planner_hook:

+   /* calc differences of buffer counters. */
+   bufusage = compute_buffer_counters(bufusage_start, 
pgBufferUsage);
+
+   /*
+* we only store planning duration, query text has been 
initialized
+* during previous pgss_post_parse_analyze as it not available 
inside
+* pgss_planner_hook.
+*/
+   pgss_store(query_text,

Do we need to calculate bufusage in here?
We only store planning duration in the following pgss_store.

--
Yoshikazu Imai

Re: Python versions (was Re: RHEL 8.0 build)

[Peter Eisentraut]

On 2019-09-07 22:18, Tom Lane wrote:
> So this subject has just intruded itself again, because new buildfarm
> member "morepork" is failing in the pre-v10 branches, because ... you
> guessed it ... it has "python3" but not "python".  This situation is
> going to get worse, not better, as time goes on, so I think we really
> should back-patch 7291733ac into all active branches.

OK with me.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

MSVC buildfarm critters are not running modules' TAP tests

[Tom Lane]

I noticed $subject while checking to see if commit db4383189's
new test script was behaving properly in the buildfarm.  dory,
for one, should be running it but it just isn't.

It looks to me like the reason is that src/tools/msvc/vcregress.pl's
subroutine subdircheck isn't considering the possibility that
subdirectories of src/test/modules contain TAP tests.  The
same code is used for contrib, so several existing TAP tests
are being missed there too.

I took a stab at fixing this, but lacking a Windows environment
to test in, I can't be sure if it works.  The attached does kinda
sorta work if I run it in a Linux environment --- but I found that
system() doesn't automatically expand "t/*.pl" on Linux.  Is that
an expected difference between Linux and Windows perl?  I hacked
around that by adding a glob() call in sub tap_check, as seen in
the first hunk below, but I'm not very sure if that hunk should
get committed or not.

For ease of review, I did not re-indent the main part of sub
subdircheck, though that needs to be done before committing.

Anybody with suitable tools care to test/commit this?

regards, tom lane

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 33d8fb5..c2f2695 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -207,7 +207,7 @@ sub tap_check
 	my $dir = shift;
 	chdir $dir;
 
-	my @args = ("prove", @flags, "t/*.pl");
+	my @args = ("prove", @flags, glob("t/*.pl"));
 
 	# adjust the environment for just this test
 	local %ENV = %ENV;
@@ -391,27 +391,24 @@ sub plcheck
 	return;
 }
 
+# Run tests in a specified subdirectory of current directory.
+# Returns 0 if OK, else exit code
 sub subdircheck
 {
 	my $module = shift;
 
-	if (   !-d "$module/sql"
-		|| !-d "$module/expected"
-		|| (!-f "$module/GNUmakefile" && !-f "$module/Makefile"))
-	{
-		return;
-	}
+	chdir($module) || return 0;
 
-	chdir $module;
-	my @tests = fetchTests();
+	my $mstat = 0;
 
-	# Leave if no tests are listed in the module.
-	if (scalar @tests == 0)
+	# Look for traditional-style regression tests.
+	if (-d "sql" && -d "expected"
+		&& (-f "GNUmakefile" || -f "Makefile"))
 	{
-		chdir "..";
-		return;
-	}
+	my @tests = fetchTests();
 
+	if (scalar @tests > 0)
+	{
 	my @opts = fetchRegressOpts();
 
 	# Special processing for python transform modules, see their respective
@@ -437,15 +434,29 @@ sub subdircheck
 	}
 
 	print "\n";
-	print "Checking $module\n";
+	print "Running $module regression tests\n";
 	my @args = (
 		"$topdir/$Config/pg_regress/pg_regress",
 		"--bindir=${topdir}/${Config}/psql",
 		"--dbname=contrib_regression", @opts, @tests);
 	print join(' ', @args), "\n";
 	system(@args);
+	my $status = $? >> 8;
+	$mstat ||= $status;
+	}
+	}
+
+	# Look for TAP tests.
+	if ($config->{tap_tests} && -d "t")
+	{
+		print "\n";
+		print "Running $module TAP tests\n";
+		my $status = tap_check(getcwd());
+		$mstat ||= $status;
+	}
+
 	chdir "..";
-	return;
+	return $mstat;
 }
 
 sub contribcheck
@@ -462,8 +473,7 @@ sub contribcheck
 		next if ($module =~ /_plpython$/ && !defined($config->{python}));
 		next if ($module eq "sepgsql");
 
-		subdircheck($module);
-		my $status = $? >> 8;
+		my $status = subdircheck($module);
 		$mstat ||= $status;
 	}
 	exit $mstat if $mstat;
@@ -476,8 +486,7 @@ sub modulescheck
 	my $mstat = 0;
 	foreach my $module (glob("*"))
 	{
-		subdircheck($module);
-		my $status = $? >> 8;
+		my $status = subdircheck($module);
 		$mstat ||= $status;
 	}
 	exit $mstat if $mstat;

Re: Python versions (was Re: RHEL 8.0 build)

[Tom Lane]

Peter Eisentraut  writes:
> On 2019-09-07 22:18, Tom Lane wrote:
>> So this subject has just intruded itself again, because new buildfarm
>> member "morepork" is failing in the pre-v10 branches, because ... you
>> guessed it ... it has "python3" but not "python".  This situation is
>> going to get worse, not better, as time goes on, so I think we really
>> should back-patch 7291733ac into all active branches.

> OK with me.

OK, done.

regards, tom lane

Re: MSVC buildfarm critters are not running modules' TAP tests

[Christoph Moench-Tegeder]

## Tom Lane (t...@sss.pgh.pa.us):

> I took a stab at fixing this, but lacking a Windows environment
> to test in, I can't be sure if it works.  The attached does kinda
> sorta work if I run it in a Linux environment --- but I found that
> system() doesn't automatically expand "t/*.pl" on Linux.  Is that
> an expected difference between Linux and Windows perl?

At least the behaviour on Linux (or any unix) is expected: if you pass
a list to perl's system(), perl does not run the command under a shell
(a shell is only invoked if there's only a scalar argument to system()
(or if the list has only one element) and that argument contains shell
metacharacters). That's a source of no small amount "fun" for perl
programms "shelling out", because "sometimes" there is no shell.
Perl's system hase some more caveats, "perldoc -f system" has a
starter on that topic.

Regards,
Christoph

-- 
Spare Space

Re: having issues with PG12 debian packaging repository

[Magnus Hagander]

On Wed, Sep 4, 2019 at 5:43 PM Murat Tuncer  wrote:

> Hello hackers
>
> I am getting sporadic errors when I tried to use PG12 bionic debian
> repository.
>
> Here is the error message that is result of apt-get update.
> ---
> Failed to fetch
> http://apt.postgresql.org/pub/repos/apt/dists/bionic-pgdg/main/binary-i386/Packages.gz
> File has unexpected size (260865 != 260866). Mirror sync in progress? [IP:
> 34.96.81.152 80]
> Hashes of expected file:
>  - Filesize:260866 [weak]
>  - SHA256:433bef097d8a54a9899350c182d0074c1a13f62c8e7e9987cc6c63cd11242abc
>  - SHA1:1be55e080a1dd277929f095690ae9b9cf01e971f [weak]
>  - MD5Sum:08189bf54aa297f53b9656bc3c529c62 [weak]
>  Release file created at: Mon, 02 Sep 2019 10:25:33 +
>  Failed to fetch
> http://apt.postgresql.org/pub/repos/apt/dists/bionic-pgdg/main/binary-amd64/Packages.gz
>  Some index files failed to download. They have been ignored, or old ones
> used instead.
> --
>
> It usually succeeds when I run it again.  Is there a way to avoid this?
>
>
This sounds very similar to an issue people ran into on travis, which I
believe was tracked down to travis putting a cache in between themselves
and apt.postgresql.org, which broke the order of downloads. Any chance you
also have a cache sitting there somewhere?

//Magnus

Re: Bug in GiST paring heap comparator

[Alexander Korotkov]

On Fri, Sep 6, 2019 at 5:44 PM Alexander Korotkov
 wrote:
> I'm going to push both if no objections.

So, pushed!

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: BUG #15977: Inconsistent behavior in chained transactions

[Peter Eisentraut]

On 2019-09-07 18:32, fn ln wrote:
>> Missed them somehow. But I don't think they're quite sufficient. I think
>> at least we also need tests that test things like multi-statement
>> exec_simple-query() *with* explicit transactions and chaining.
> Added a few more tests for that.

I committed this patch with some cosmetic changes and documentation updates.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] kNN for btree

[Alexander Korotkov]

On Tue, Sep 3, 2019 at 2:19 AM Alvaro Herrera  wrote:
>
> On 2019-Sep-03, Alexander Korotkov wrote:
>
> > I think patches 0001-0008 are very clear and extends our index-AM
> > infrastructure in query straightforward way.  I'm going to propose
> > them for commit after some further polishing.
>
> Hmm.  Why is 0001 needed?  I see that 0005 introduces a call to that
> function, but if attnum == 0 then it doesn't call it.  Maybe it was
> necessary in an older version of the patch?

Regarding "attno >= 1" check I agree with you.  It should be changed
to assert.  But "attno <= rd_index->indnkeyatts" check appears to be
needed for current code already.  It appears that gistproperty() can
ask get_index_column_opclass() for non-key attribute.  Then
get_index_column_opclass()  returns garbage past oidvector value.
Typically get_opclass_opfamily_and_input_type() doesn't find this
garbage opclass oid and gistproperty() returns null as expected.  But
this is bug and needs to be fixed.

I'm going to push 0001 changing "attno >= 1" to assert.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: MSVC buildfarm critters are not running modules' TAP tests

[Andrew Dunstan]

On 9/8/19 12:07 PM, Tom Lane wrote:
> I noticed $subject while checking to see if commit db4383189's
> new test script was behaving properly in the buildfarm.  dory,
> for one, should be running it but it just isn't.
>
> It looks to me like the reason is that src/tools/msvc/vcregress.pl's
> subroutine subdircheck isn't considering the possibility that
> subdirectories of src/test/modules contain TAP tests.  The
> same code is used for contrib, so several existing TAP tests
> are being missed there too.
>
> I took a stab at fixing this, but lacking a Windows environment
> to test in, I can't be sure if it works.  The attached does kinda
> sorta work if I run it in a Linux environment --- but I found that
> system() doesn't automatically expand "t/*.pl" on Linux.  Is that
> an expected difference between Linux and Windows perl?  I hacked
> around that by adding a glob() call in sub tap_check, as seen in
> the first hunk below, but I'm not very sure if that hunk should
> get committed or not.
>
> For ease of review, I did not re-indent the main part of sub
> subdircheck, though that needs to be done before committing.
>
> Anybody with suitable tools care to test/commit this?
>
>   



Actually, I think vcregress.pl is OK, this is a gap in the buildfarm
client's coverage that will be fixed when I make a new release. Any day
now I hope. See



bowerbird which is already running that code is running the test you
refer to:



cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgsql: Use data directory inode number, not port, to select SysV resour

[Andrew Dunstan]

On 9/6/19 3:51 PM, Andrew Dunstan wrote:
> On 9/6/19 2:42 PM, Tom Lane wrote:
>> Andrew Dunstan  writes:
>>> Given your stated intention, I think the simplest way to get it is just
>>> this, without worrying about what the perl modules might do:
>>> -if ($@)
>>> +if ($@ || $windows_os)
>> WFM, do you want to push that?
>>
>>  
>
> done.
>
>

[redirected to -hackers]


I'm going to disable this test (src/test/recovery/t/017_shm.pl) on
Windows on the back branches too unless there's a violent objection. The
reason is that the script runs "postgres --single" and that fails on
Windows when run by an administrative account. We've carefully enabled
postgres and its tests to run safely under an admin account. I
discovered this as part of my myss2 testing.


cheers


andrew

Re: MSVC buildfarm critters are not running modules' TAP tests

[Tom Lane]

Andrew Dunstan  writes:
> On 9/8/19 12:07 PM, Tom Lane wrote:
>> It looks to me like the reason is that src/tools/msvc/vcregress.pl's
>> subroutine subdircheck isn't considering the possibility that
>> subdirectories of src/test/modules contain TAP tests.  The
>> same code is used for contrib, so several existing TAP tests
>> are being missed there too.

> Actually, I think vcregress.pl is OK, this is a gap in the buildfarm
> client's coverage that will be fixed when I make a new release.

Hm.  Changing the buildfarm script would be an alternative way to
fix the issue so far as the buildfarm is concerned, but it doesn't
seem like it provides any useful way for one to manually invoke
the tests on Windows.  Or am I missing something about how that's
usually done?

regards, tom lane

Re: pgsql: Use data directory inode number, not port, to select SysV resour

[Tom Lane]

Andrew Dunstan  writes:
> I'm going to disable this test (src/test/recovery/t/017_shm.pl) on
> Windows on the back branches too unless there's a violent objection.

As I said before, I think that test does nothing useful unless SysV
shmem is in use, so I see no reason not to disable it on Windows.

regards, tom lane

msys2 vs pg_upgrade/test.sh

[Andrew Dunstan]

Diagnosing this took quite a lot of time and detective work. For some
reason I don't quite understand, when calling the Windows command
processor in a modern msys2/WindowsServer2019 installation, you need to
double the slash, thus:


    cmd //c foo.bat


Some Internet postings at least seem to suggest this is by design. (FSVO
"design")


I tested this on older versions and the change appears to work, so I
propose to apply the attached patch.


This is the last obstacle I have to declaring msys2 fully supportable.



cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index c9de4f8a7e..2d78f63f5e 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -244,8 +244,11 @@ esac
 
 pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
 
+# modern versions of msys/Windows require a double slash when calling
+# "cmd /c". This also works on older versions.
+
 case $testhost in
-	MINGW*)	cmd /c analyze_new_cluster.bat ;;
+	MINGW*)	cmd //c analyze_new_cluster.bat ;;
 	*)		sh ./analyze_new_cluster.sh ;;
 esac
 
@@ -258,7 +261,7 @@ if [ -n "$pg_dumpall2_status" ]; then
 fi
 
 case $testhost in
-	MINGW*)	cmd /c delete_old_cluster.bat ;;
+	MINGW*)	cmd //c delete_old_cluster.bat ;;
 	*)	sh ./delete_old_cluster.sh ;;
 esac
 

Re: MSVC buildfarm critters are not running modules' TAP tests

[Andrew Dunstan]

On 9/8/19 5:59 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 9/8/19 12:07 PM, Tom Lane wrote:
>>> It looks to me like the reason is that src/tools/msvc/vcregress.pl's
>>> subroutine subdircheck isn't considering the possibility that
>>> subdirectories of src/test/modules contain TAP tests.  The
>>> same code is used for contrib, so several existing TAP tests
>>> are being missed there too.
>> Actually, I think vcregress.pl is OK, this is a gap in the buildfarm
>> client's coverage that will be fixed when I make a new release.
> Hm.  Changing the buildfarm script would be an alternative way to
> fix the issue so far as the buildfarm is concerned, but it doesn't
> seem like it provides any useful way for one to manually invoke
> the tests on Windows.  Or am I missing something about how that's
> usually done?
>
>   


The invocation is:


vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory


directory needs to be relative to $topdir, so something like:


vcregress.pl taptest src/test/modules/test_misc


cheers


andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: MSVC buildfarm critters are not running modules' TAP tests

[Tom Lane]

Andrew Dunstan  writes:
> On 9/8/19 5:59 PM, Tom Lane wrote:
>> Hm.  Changing the buildfarm script would be an alternative way to
>> fix the issue so far as the buildfarm is concerned, but it doesn't
>> seem like it provides any useful way for one to manually invoke
>> the tests on Windows.  Or am I missing something about how that's
>> usually done?

> The invocation is:
> vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory

Sure, I saw that you can run one test that way ... but what do you
do when you want the equivalent of check-world?

(I'm surprised that vcregress hasn't already got a "world" option.
But at least there should be a determinate list of what you need
to run to get all the tests.)

regards, tom lane

Re: MSVC buildfarm critters are not running modules' TAP tests

[Michael Paquier]

On Sun, Sep 08, 2019 at 06:18:33PM -0400, Tom Lane wrote:
> Sure, I saw that you can run one test that way ... but what do you
> do when you want the equivalent of check-world?

I think that it is a good idea to add in subdircheck an extra path to
check after TAP tests and run optionally these on top of the normal
regression tests.  I have a couple of comments.

+   # Look for TAP tests.
+   if ($config->{tap_tests} && -d "t")
+   {
+   print
"\n";
+   print "Running $module TAP tests\n";
+   my $status = tap_check(getcwd());
+   $mstat ||= $status;
+   }
Shouldn't we check after TAP_TESTS in the Makefile?

There is an argument to also check after isolation tests and run
them.  It seems to me that we should check after ISOLATION, and run
optionally the tests if there is anything present.  So we need
something like fetchTests() and fetchRegressOpts() but for isolation
tests.

The glob() part is a good idea in itself I think.  Why not
back-patching it?  I could double-check it as well.
--
Michael


signature.asc
Description: PGP signature

Re: MSVC buildfarm critters are not running modules' TAP tests

[Tom Lane]

Michael Paquier  writes:
> I think that it is a good idea to add in subdircheck an extra path to
> check after TAP tests and run optionally these on top of the normal
> regression tests.  I have a couple of comments.

> Shouldn't we check after TAP_TESTS in the Makefile?

Yeah, perhaps, but I wasn't sure about how to do that easily.
Feel free to add it ...

> There is an argument to also check after isolation tests and run
> them.

Hm, yeah, if there are any such tests in those directories.

> The glob() part is a good idea in itself I think.  Why not
> back-patching it?  I could double-check it as well.

The whole thing should be back-patched into branches that have
any affected tests.  (But, please, not till after beta4 is
tagged.)

regards, tom lane

Re: MSVC buildfarm critters are not running modules' TAP tests

[Andrew Dunstan]

On 9/8/19 6:18 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 9/8/19 5:59 PM, Tom Lane wrote:
>>> Hm.  Changing the buildfarm script would be an alternative way to
>>> fix the issue so far as the buildfarm is concerned, but it doesn't
>>> seem like it provides any useful way for one to manually invoke
>>> the tests on Windows.  Or am I missing something about how that's
>>> usually done?
>> The invocation is:
>> vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory
> Sure, I saw that you can run one test that way ... but what do you
> do when you want the equivalent of check-world?
>
> (I'm surprised that vcregress hasn't already got a "world" option.
> But at least there should be a determinate list of what you need
> to run to get all the tests.)
>
>   



Possibly. The script has not had as much love as it needs.


cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: MSVC buildfarm critters are not running modules' TAP tests

[Michael Paquier]

On Sun, Sep 08, 2019 at 07:44:16PM -0400, Tom Lane wrote:
> Yeah, perhaps, but I wasn't sure about how to do that easily.
> Feel free to add it ...

Feeding the makefile contents and then doing a lookup using =~ should
be enough.  I think that we should just refactor set of routines for
fetchTests() so as it uses the flag to look after as input.
fetchRegressOpts() has tweaks for ENCODING and NO_LOCALE which don't
apply to the isolation tests so perhaps a different routine would be
better.

> Hm, yeah, if there are any such tests in those directories.

snapshot_too_old is a good example to work with.

> The whole thing should be back-patched into branches that have
> any affected tests.  (But, please, not till after beta4 is
> tagged.)

Sure.  Don't worry about that.  I am focused on another thing lately
and it does not touch back-branches.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] pageinspect function to decode infomasks

[Amit Kapila]

On Sun, Sep 8, 2019 at 1:06 PM Amit Kapila  wrote:
>
> On Thu, Sep 5, 2019 at 2:17 PM Masahiko Sawada  wrote:
> >
> > Thanks. I hope the attached new patch fixes this issue.
> >
>

Some more comments:
*
+SELECT t_infomask, t_infomask2, flags
+FROM heap_page_items
(get_raw_page('test1', 0)),
+ LATERAL heap_infomask_flags(t_infomask,
t_infomask2, true) m(flags);

Do we really need a LATERAL clause in the above kind of queries?
AFAIU, the functions can reference the columns that exist in the FROM
list, but I might be missing some point.

*
+Datum
+heap_infomask_flags(PG_FUNCTION_ARGS)
+{
+ uint16 t_infomask = PG_GETARG_INT16(0);
+ uint16 t_infomask2 = PG_GETARG_INT16(1);
+ bool decode_combined = PG_GETARG_BOOL(2);
+ int cnt = 0;
+ ArrayType *a;
+ int bitcnt;
+ Datum *d;
+
+ bitcnt = pg_popcount((const char *) &t_infomask, sizeof(uint16)) +
+ pg_popcount((const char *) &t_infomask2, sizeof(uint16));

All the functions in this file are allowed only for superusers and
there is an explanation for the same as mentioned in the file header
comments.  Is there a reason for this function to be different?

I think one possible explanation could be that here we are not passing
raw page on which this function will operate on, but isn't the same
true for tuple_data_split?

In any case, if you think that this function needs to behave
differently w.r.t superuser privileges, then it is better to add some
comments in the function header to explain the same.

*
+Datum
+heap_infomask_flags(PG_FUNCTION_ARGS)
{
..
+
+ d = (Datum *) palloc0(sizeof(Datum) * bitcnt);

It seems we don't free this memory before leaving this function.  I
think it shouldn't be a big problem as this will be normally allocated
in ExprContext and shouldn't last for long, but if there is no strong
reason, I think it is better to free it.  You can find the examples in
code both where we free after such usage and where we don't.  I prefer
to free it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: BUG #15977: Inconsistent behavior in chained transactions

[fn ln]

Confirmed. Thank you all for your help.

The only concern is that this test:

   SET TRANSACTION READ WRITE\; COMMIT AND CHAIN;  -- error
   SHOW transaction_read_only;

   SET TRANSACTION READ WRITE\; ROLLBACK AND CHAIN;  -- error
   SHOW transaction_read_only;

makes more sense with READ ONLY because default_transaction_read_only is
off at this point.

2019年9月9日(月) 5:27 Peter Eisentraut :

> On 2019-09-07 18:32, fn ln wrote:
> >> Missed them somehow. But I don't think they're quite sufficient. I think
> >> at least we also need tests that test things like multi-statement
> >> exec_simple-query() *with* explicit transactions and chaining.
> > Added a few more tests for that.
>
> I committed this patch with some cosmetic changes and documentation
> updates.
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: refactoring - share str2*int64 functions

[Michael Paquier]

On Thu, Sep 05, 2019 at 03:52:48PM +0900, Michael Paquier wrote:
> Right, there was this part.  This brings also the point of having one
> interface for the backend as all the error messages for the backend
> are actually the same, with the most simple name being that:
> pg_strtoint(value, size, error_ok).

I have been looking at that for the last couple of days.  First, I
have consolidated all the error strings in a single routine like this
one, except that error_ok is not really needed if you take this
approach: callers that don't care about failures could just call the
set of routines in common/string.c and be done with it.

Attached is an update of my little module that I used to check that
the refactoring was done correctly (regression tests attached), it
also includes a couple of routines to check the performance difference
between one approach and the other, with focus on two things:
- Is pg_strtouint64 really improved?
- How much do we lose by moving to a common interface in the backend
with pg_strtoint?

The good news is that removing strtol from pg_strtouint64 really
improves the performance as already reported, and with one billion
calls in a tight loop you see a clear difference:
=# select pg_strtouint64_old_check('1', 10);
 pg_strtouint64_old_check
--
1
(1 row)
Time: 15576.539 ms (00:15.577)
=# select pg_strtouint64_new_check('1', 10);
 pg_strtouint64_new_check
--
1
(1 row)
Time: 8820.544 ms (00:08.821)

So the new implementation is more than 40% faster with this
micro-benchmark on my Debian box.

The bad news is that a pg_strtoint() wrapper is not a good idea:
=# select pg_strtoint_check('1', 10, 4);
 pg_strtoint_check
---
 1
(1 row)
Time: 11178.101 ms (00:11.178)
=# select pg_strtoint32_check('1', 10);
 pg_strtoint32_check
-
   1
(1 row)
Time: 9252.894 ms (00:09.253)
So trying to consolidate all error messages leads to a 15% hit with
this test, which sucks.

So, it seems to me that if we want to have a consolidation of
everything, we basically need to have the generic error messages for
the backend directly into common/string.c where the routines are
refactored, and I think that we should do the following based on the
patch attached:
- Just remove pg_strtoint(), and move the error generation logic in
common/string.c.
- Add an error_ok flag for all the pg_strto[u]int{16|32|64} routines,
which generates errors only for FRONTEND is not defined.
- Use pg_log_error() for the frontend errors.

If those errors are added everywhere, we would basically have no code
paths in the frontend or the backend (for the unsigned part only)
which use them yet.  Another possibility would be to ignore the
error_ok flag in those cases, but that's inconsistent.  My take would
be here to have a more generic error message, like that:
"value \"%s\" is out of range for [unsigned] {16|32|64}-bit integer" 
"invalid input syntax for [unsigned] {16|32|64}-bit integer: \"%s\"\n"

I do not suggest to change the messages for the backend for signed
entries though, as these are linked to the data types used.

Attached is an update of my toy module, and an updated patch with what
I have done up to now.  This stuff already does a lot, so for now I
have not worked on the removal strtoint() in string.c yet.  We could
just do that with a follow-up patch and make it use the new conversion
routines once we are sure that they are in a correct shape.  As
strtoint() makes use of strtol(), switching to the new routines will
be much faster anyway...

Fabien, any thoughts?
--
Michael
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 221b47298c..28891ba337 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -79,6 +79,7 @@
 #include "utils/builtins.h"
 #include "utils/hashutils.h"
 #include "utils/memutils.h"
+#include "common/string.h"
 
 PG_MODULE_MAGIC;
 
@@ -1022,7 +1023,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 		/* parse command tag to retrieve the number of affected rows. */
 		if (completionTag &&
 			strncmp(completionTag, "COPY ", 5) == 0)
-			rows = pg_strtouint64(completionTag + 5, NULL, 10);
+			(void) pg_strtouint64(completionTag + 5, &rows);
 		else
 			rows = 0;
 
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index adf0490f85..9583845d7e 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -306,7 +306,7 @@ check_foreign_key(PG_FUNCTION_ARGS)
 		/* internal error */
 		elog(ERROR, "check_foreign_key: too short %d (< 5) list of arguments", nargs);
 
-	nrefs = pg_strtoint32(args[0]);
+	nrefs = pg_strtoint(args[0], 4);
 	if (nrefs < 1)
 		/* internal error */
 		elog(ERROR, "check_foreign_key: %d (< 1) number of references specified", nrefs);

RE: [PATCH] Speedup truncates of relation forks

[Jamison, Kirk]

On Friday, September 6, 2019 11:51 PM (GMT+9), Alvaro Herrera wrote:

Hi Alvaro,
Thank you very much for the review!

> On 2019-Sep-05, Jamison, Kirk wrote:
> 
> > I also mentioned it from my first post if we can just remove this dead code.
> > If not, it would require to modify the function because it would also
> > need nforks as input argument when calling DropRelFileNodeBuffers. I
> > kept my changes in the latest patch.
> > So should I remove the function now or keep my changes?
> 
> Please add a preliminary patch that removes the function.  Dead code is good,
> as long as it is gone.  We can get it pushed ahead of the rest of this.

Alright. I've attached a separate patch removing the smgrdounlinkfork.


> What does it mean to "mark" a dirty page in FSM?  We don't have the concept
> of marking pages as far as I know (and I don't see that the patch does any
> sort of mark).  Do you mean to find where it is and return its block number?

Yes. "Mark" is probably not a proper way to describe it, so I temporarily
changed it to "locate" and renamed the function to FreeSpaceMapLocateBlock().
If anyone could suggest a more appropriate name, that'd be appreciated.


> If so, I wonder how this handles concurrent table extension: are we keeping
> some sort of lock that prevents it?
> (... or would we lose any newly added pages that receive tuples while this
> truncation is ongoing?)

I moved the the description about acquiring AccessExclusiveLock
from FreeSpaceMapLocateBlock() and visibilitymap_truncate_prepare() to the
smgrtruncate description instead.
IIUC, in lazy_truncate_heap() we still acquire AccessExclusiveLock for the 
relation
before calling RelationTruncate(), which then calls smgrtruncate().
While holding the exclusive lock, the following are also called to check
if rel has not extended and verify that end pages contain no tuples while
we were vacuuming (with non-exclusive lock).
  new_rel_pages = RelationGetNumberOfBlocks(onerel);
  new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
I assume that the above would update the correct number of pages.
We then release the exclusive lock as soon as we have truncated the pages.


> I think the new API of smgrtruncate() is fairly confusing.  Would it be better
> to define a new struct { ForkNum forknum; BlockNumber blkno; } and pass an
> array of those?

This is for readbility, right? However, I think there's no need to define a
new structure for it, so I kept my changes.
  smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber 
*nblocks).
I also declared *forkNum and nforks next to each other as suggested by 
Sawada-san.


What do you think about these changes?


Regards,
Kirk Jamison


v1-0001-Remove-deadcode-smgrdounlinkfork.patch
Description: v1-0001-Remove-deadcode-smgrdounlinkfork.patch


v7-0001-Speedup-truncates-of-relation-forks.patch
Description: v7-0001-Speedup-truncates-of-relation-forks.patch