Re: GiST VACUUM

[Thomas Munro]

On Tue, Jul 24, 2018 at 6:04 AM, Andrey Borodin  wrote:
>> 21 июля 2018 г., в 17:11, Andrey Borodin  написал(а):
>> <0001-Physical-GiST-scan-in-VACUUM-v13.patch>
>
> Just in case, here's second part of patch series with actual page deletion.

Hi Andrey,

Cfbot says:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.7146

That's because you declared a new variable after some other
statements.  You can't do that in old school C89.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/409401951

That segfaulted here:

#0 0x004ab620 in setinternal (state=,
state=, blkno=368) at gistvacuum.c:44
44 state->internalPagesMap[blkno / 8] |= 1 << (blkno % 8);

internalPagesMap was NULL, or pointed to memory that was too small and
happened to be near an unmapped region (unlikely?), or was some other
corrupted address?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Covering GiST indexes

[Thomas Munro]

On Fri, Jul 6, 2018 at 5:27 AM, Andrey Borodin  wrote:
> Here is v3 version of the patch. I've fixed some comments and added some 
> words to docs.

Hi again Andrey,

Cfbot reported the following difference (twice) in the
index_including_gist test:

- ERROR: included columns must not intersect with key columns
+ ERROR: relation "tbl_gist_idx" already exists

Well, both of those errors are valid complaints in this case... maybe
the order of checks changed in master since you wrote the patch?
Probably better to use a different name for the index anyway.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Thomas Munro]

On Thu, Mar 1, 2018 at 3:24 AM, Ivan Kartyshov
 wrote:
> Could you give me your ideas over these patches.

Hi Ivan,

Not sure if this is expected at this stage or not, but just in case
you didn't know... the tests under src/test/subscription seem to be
broken:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/409358734

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 03:54 AM, Tomas Vondra wrote:
> 
> 
> On 07/27/2018 10:20 AM, Kyotaro HORIGUCHI wrote:
>> Thank you for taking this.
>>
>> At Thu, 26 Jul 2018 17:12:50 +0200, Tomas Vondra 
>>  wrote in 
>> <672f4c42-6742-c1ec-e9a4-1994b815a...@2ndquadrant.com>
>>> On 07/11/2018 07:13 PM, Emre Hasegeli wrote:
 New versions are attached after the  patch got in.  I noticed
 tests failing on Windows [1] and added alternative .out file.
 [1]
 https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.5235

>>>
>>> The version posted about two weeks ago is slightly broken - AFAICS the
>>> float.h includes in geo_ops.c and gistproc.c need to be part of 0002,
>>> not 0003. Attached is a version fixing that.
>>>
>>> Barring objections, I'll get this committed over the next few days,
>>> once I review all the individual parts once more. I'm planning to
>>> commit the 6 parts separately, as submitted. No backpatching, as
>>> discussed before.
>>
>> +1 for no backpatching, and not merging into single big patch.
>>
> 
> I've committed the first two parts, after a review and testing.
> 
> As these two parts were primarily refactoring (and quite extensive),
> this seems like a good place to wait if the buildfarm is happy with it.
> If yes, I'll continue applying the patches that do fix/change the
> behavior in various ways.
> 

Seems there's a couple of failures like this, so far:

*** 42,48 
s
  -
   {1,-1,1}
!  {1,-1,0}
   {-0.4,-1,-6}
   {-0.000184615384615385,-1,15.3846153846154}
   {1,-1,11}
--- 42,48 
s
  -
   {1,-1,1}
!  {1,-1,-0}
   {-0.4,-1,-6}
   {-0.000184615384615385,-1,15.3846153846154}
   {1,-1,11}
***

It's always 0/-0 difference, and it's limited to power machines. I'll
try to get access to such system and see what's wrong.

regards

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

Re: Removing unneeded self joins

[Thomas Munro]

On Sat, Jul 28, 2018 at 12:26 AM, Alexander Kuzmenkov
 wrote:
> Here is a current version of the patch, still rather experimental.

Hi Alexander,

The eval-qual-plan isolation test is failing:

- checking   1050   checking   600
+ checking   600checking   600

That's the result of a self join with EPQ on one side of the join:

SELECT * FROM accounts a1, accounts a2
  WHERE a1.accountid = a2.accountid
  FOR UPDATE OF a1;

I think you need to disable the optimisation when there is a locking
clause on one side.  Maybe it could be allowed if it's on both sides?
Not sure.

+ Assert(is_opclause(rinfo->clause));
+ Expr *leftOp = (Expr *) get_leftop(rinfo->clause);

You can't declare a variable here in C89.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Covering GiST indexes

[Andrey Borodin]

Hi, Thomas!29 июля 2018 г., в 14:28, Thomas Munro  написал(а):On Fri, Jul 6, 2018 at 5:27 AM, Andrey Borodin  wrote:Here is v3 version of the patch. I've fixed some comments and added some words to docs.Hi again Andrey,Cfbot reported the following difference (twice) in theindex_including_gist test:- ERROR: included columns must not intersect with key columns+ ERROR: relation "tbl_gist_idx" already existsWell, both of those errors are valid complaints in this case... maybethe order of checks changed in master since you wrote the patch?Probably better to use a different name for the index anyway.Thanks! The problem appeared with commit 701fd0b [0] which dropped validation rules checked in failed test.  Here's the patch with fixed tests.Best regards, Andrey Borodin.[0] https://github.com/postgres/postgres/commit/701fd0bbc98fe8211d36e96f90753985104cd295

0001-Covering-GiST-v4.patch
Description: Binary data

Re: [PATCH] Improve geometric types

[Thomas Munro]

On Sun, Jul 29, 2018 at 10:35 PM, Tomas Vondra
 wrote:
> It's always 0/-0 difference, and it's limited to power machines. I'll
> try to get access to such system and see what's wrong.

This is suspicious:

/* on some platforms, the preceding expression tends to produce -0 */
if (line->C == 0.0)
line->C = 0.0;

FWIW I tried this on our Linux/POWER8 box but it didn't show the problem.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [PATCH] Improve geometric types

[Thomas Munro]

On Sun, Jul 29, 2018 at 10:57 PM, Thomas Munro
 wrote:
> On Sun, Jul 29, 2018 at 10:35 PM, Tomas Vondra
>  wrote:
>> It's always 0/-0 difference, and it's limited to power machines. I'll
>> try to get access to such system and see what's wrong.
>
> This is suspicious:
>
> /* on some platforms, the preceding expression tends to produce -0 */
> if (line->C == 0.0)
> line->C = 0.0;

I mean, it's suspiciously absent from the new line_construct()
function.  It was introduced here:

commit 43fe90f66a0b200f6c32507428349afb45f661ca
Author: Tom Lane 
Date:   Fri Oct 25 15:55:15 2013 -0400

Suppress -0 in the C field of lines computed by line_construct_pts().

It's not entirely clear why some PPC machines are generating -0 here, since
the underlying computation should be exactly 0 - 0.  Perhaps there's some
wider-than-nominal-precision calculations happening?  Anyway, the best way
to avoid platform-dependent results seems to be to explicitly reset -0 to
regular zero.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: GiST VACUUM

[Andrey Borodin]

Hi!

Thank you!

> 29 июля 2018 г., в 14:04, Thomas Munro  
> написал(а):
> 
> On Tue, Jul 24, 2018 at 6:04 AM, Andrey Borodin  wrote:
>>> 21 июля 2018 г., в 17:11, Andrey Borodin  написал(а):
>>> <0001-Physical-GiST-scan-in-VACUUM-v13.patch>
>> 
>> Just in case, here's second part of patch series with actual page deletion.
> 
> Hi Andrey,
> 
> Cfbot says:
> 
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.7146
> 
> That's because you declared a new variable after some other
> statements.  You can't do that in old school C89.
Yep, mismerged patch steps and created misplaced declaration

> https://travis-ci.org/postgresql-cfbot/postgresql/builds/409401951
> 
> That segfaulted here:
> 
> #0 0x004ab620 in setinternal (state=,
> state=, blkno=368) at gistvacuum.c:44
> 44 state->internalPagesMap[blkno / 8] |= 1 << (blkno % 8);
> 
> internalPagesMap was NULL, or pointed to memory that was too small and
> happened to be near an unmapped region (unlikely?), or was some other
> corrupted address?
Yes, there was conditionally uninitialized variable mapNumPages. I do not know 
why it didn't trigger failure last time, but now it was reproduced on my 
machine reliably.

Fixed both problems. PFA v14.

Best regards, Andrey Borodin.


0002-Delete-pages-during-GiST-VACUUM-v14.patch
Description: Binary data


0001-Physical-GiST-scan-in-VACUUM-v14.patch
Description: Binary data

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 01:28 PM, Thomas Munro wrote:
> On Sun, Jul 29, 2018 at 10:57 PM, Thomas Munro
>  wrote:
>> On Sun, Jul 29, 2018 at 10:35 PM, Tomas Vondra
>>  wrote:
>>> It's always 0/-0 difference, and it's limited to power machines. I'll
>>> try to get access to such system and see what's wrong.
>>
>> This is suspicious:
>>
>> /* on some platforms, the preceding expression tends to produce -0 */
>> if (line->C == 0.0)
>> line->C = 0.0;
> 
> I mean, it's suspiciously absent from the new line_construct()
> function.  It was introduced here:
> 
> commit 43fe90f66a0b200f6c32507428349afb45f661ca
> Author: Tom Lane 
> Date:   Fri Oct 25 15:55:15 2013 -0400
> 
> Suppress -0 in the C field of lines computed by line_construct_pts().
> 
> It's not entirely clear why some PPC machines are generating -0 here, 
> since
> the underlying computation should be exactly 0 - 0.  Perhaps there's some
> wider-than-nominal-precision calculations happening?  Anyway, the best way
> to avoid platform-dependent results seems to be to explicitly reset -0 to
> regular zero.
> 

Hmm, I see. I think adding it to the else branch should do the trick,
then, I guess. But I'd be much happier if I could test it somewhere
before the commit.

regards

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

Re: [PATCH] Improve geometric types

[Jeff Janes]

On Sat, Jul 28, 2018 at 9:54 PM, Tomas Vondra 
wrote:

>
>
> I've committed the first two parts, after a review and testing.
>
>
I'm getting a compiler warning now:

geo_ops.c: In function 'line_closept_point':
geo_ops.c:2528:7: warning: variable 'retval' set but not used
[-Wunused-but-set-variable]
  bool  retval;


gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.10)

Cheers,

Jeff


> As these two parts were primarily refactoring (and quite extensive),
> this seems like a good place to wait if the buildfarm is happy with it.
> If yes, I'll continue applying the patches that do fix/change the
> behavior in various ways.
>
>
> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

Re: add verbosity to pg_basebackup for sync

[Jeff Janes]

On Fri, Jul 27, 2018 at 7:10 PM, Michael Paquier 
wrote:

> On Fri, Jul 27, 2018 at 11:58:42AM -0400, Jeff Janes wrote:
> > But it was really waiting for the syncs of the new -D dir to finish.  The
> > attached patch adds a -v notice that it is starting to do the sync, with
> > the wording taken from initdb's equivalent message.
>
> This is a good idea.  Would we want that back-patched?  I am sure that
> nobody is going to complain for an extra informational log.
>

Thanks for committing it.  It is probably safer not to backpatch, as
someone might have scripts that look at the output of -v so it shouldn't
change between minor releases.  I don't think looking at those would be a
good idea, but they may have their reasons.

Cheers,

Jeff

Re: Inconsistent error message in bt_page_items_bytea().

[Michael Paquier]

On Fri, Jul 27, 2018 at 05:13:37PM +0530, Ashutosh Sharma wrote:
> All the pageinspect functions dealing with raw page has the error
> message as "must be superuser to use raw page function" however,
> that's not true for bt_page_items_bytea() which has  "must be
> superuser to use pageinspect functions". This seems to me like a copy
> paste error which got transferred from bt_page_items (the function
> that doesn't deal with raw page).

Agreed.  get_raw_page_internal() mentions "must be superuser to use raw
functions" which is inconsistent with the rest, so I fixed this second
one and pushed.  Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: add verbosity to pg_basebackup for sync

[Michael Paquier]

On Sun, Jul 29, 2018 at 10:45:10AM -0400, Jeff Janes wrote:
> Thanks for committing it.  It is probably safer not to backpatch, as
> someone might have scripts that look at the output of -v so it shouldn't
> change between minor releases.  I don't think looking at those would be a
> good idea, but they may have their reasons.

Yes, I was wondering about this possibility as well.  That's unlikely so
but..  Impossible to be sure.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 04:31 PM, Jeff Janes wrote:
> 
> 
> On Sat, Jul 28, 2018 at 9:54 PM, Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> 
> 
> I've committed the first two parts, after a review and testing.
> 
> 
> I'm getting a compiler warning now:
> 
> geo_ops.c: In function 'line_closept_point':
> geo_ops.c:2528:7: warning: variable 'retval' set but not used
> [-Wunused-but-set-variable]
>   bool  retval;
>  

Yeah, the variable is apparently only used in an assert. Will fix.

regards

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

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 02:03 PM, Tomas Vondra wrote:
> 
> 
> On 07/29/2018 01:28 PM, Thomas Munro wrote:
>> On Sun, Jul 29, 2018 at 10:57 PM, Thomas Munro
>>  wrote:
>>> On Sun, Jul 29, 2018 at 10:35 PM, Tomas Vondra
>>>  wrote:
 It's always 0/-0 difference, and it's limited to power machines. I'll
 try to get access to such system and see what's wrong.
>>>
>>> This is suspicious:
>>>
>>> /* on some platforms, the preceding expression tends to produce -0 
>>> */
>>> if (line->C == 0.0)
>>> line->C = 0.0;
>>
>> I mean, it's suspiciously absent from the new line_construct()
>> function.  It was introduced here:
>>
>> commit 43fe90f66a0b200f6c32507428349afb45f661ca
>> Author: Tom Lane 
>> Date:   Fri Oct 25 15:55:15 2013 -0400
>>
>> Suppress -0 in the C field of lines computed by line_construct_pts().
>>
>> It's not entirely clear why some PPC machines are generating -0 here, 
>> since
>> the underlying computation should be exactly 0 - 0.  Perhaps there's some
>> wider-than-nominal-precision calculations happening?  Anyway, the best 
>> way
>> to avoid platform-dependent results seems to be to explicitly reset -0 to
>> regular zero.
>>
> 
> Hmm, I see. I think adding it to the else branch should do the trick,
> then, I guess. But I'd be much happier if I could test it somewhere
> before the commit.
> 

FWIW I think this should fix it. Can someone with access to an affected
machine confirm?


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 1fb2ff2603..fefb03ee86 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -1024,6 +1024,9 @@ line_construct(LINE *result, Point *pt, float8 m)
 		result->A = m;
 		result->B = -1.0;
 		result->C = pt->y - m * pt->x;
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (line->C == 0.0)
+			line->C = 0.0;
 	}
 }
 

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 05:14 PM, Tomas Vondra wrote:
> On 07/29/2018 02:03 PM, Tomas Vondra wrote:
>>
>> ...
>>
>> Hmm, I see. I think adding it to the else branch should do the trick,
>> then, I guess. But I'd be much happier if I could test it somewhere
>> before the commit.
>>
> 
> FWIW I think this should fix it. Can someone with access to an affected
> machine confirm?
> 

Gah, shouldn't have posted before trying to compile it. Here is a fixed
version of the fix.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 1fb2ff2603..621b5c33ef 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -1024,6 +1024,9 @@ line_construct(LINE *result, Point *pt, float8 m)
 		result->A = m;
 		result->B = -1.0;
 		result->C = pt->y - m * pt->x;
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (result->C == 0.0)
+			result->C = 0.0;
 	}
 }
 

Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

[Bossart, Nathan]

On 7/27/18, 7:10 PM, "Michael Paquier"  wrote:
> No problem.  If there are no objections, I am going to fix the REINDEX
> issue first and back-patch.  Its patch is the least invasive of the
> set.

This seems like a reasonable place to start.  I took a closer look at
0003.

+   /*
+* We allow the user to reindex a table if he is superuser, the 
table
+* owner, or the database owner (but in the latter case, only 
if it's not
+* a shared relation).
+*/
+   if (!(pg_class_ownercheck(relid, GetUserId()) ||
+ (pg_database_ownercheck(MyDatabaseId, GetUserId()) &&
+  !classtuple->relisshared)))
+   continue;

This is added to ReindexMultipleTables(), which is used for REINDEX
SCHEMA, REINDEX SYSTEM, and REINDEX DATABASE.  Presently, REINDEX
SCHEMA requires being the owner of the schema, and REINDEX SYSTEM and
REINDEX DATABASE require being the owner of the database.  So, if a
role is an owner of a database or the pg_catalog schema, they are able
to reindex shared catalogs like pg_authid.

This patch changes things so that only superusers or the owner of the
table can reindex shared relations.  This works as expected for
REINDEX SYSTEM and REINDEX DATABASE, but I think there is a problem
for REINDEX SCHEMA.  If the user is not the owner of the database but
is the owner of the schema, they will not be able to use REINDEX
SCHEMA to reindex any tables that they do not own.  To fix this, we
will likely need to use pg_namespace_ownercheck() instead of
pg_database_ownercheck() for the REINDEX_OBJECT_SCHEMA case.

I also noticed that this patch causes shared relations to be skipped
silently.  Perhaps we should emit a WARNING or DEBUG message when this
happens, at least for REINDEXOPT_VERBOSE.

Reindexing a single index or table requires being the owner of that
index or table.  Reindexing a database requires being the owner of
the database (note that the owner can therefore rebuild indexes of
-   tables owned by other users).  Of course, superusers can always
+   tables owned by other users).  Reindexing a shared catalog requires
+   being the owner of that shared catalog.  Of course, superusers can always
reindex anything.

I noticed that there is no mention that the owner of a schema can do
REINDEX SCHEMA, which seems important to note.  Also, the proposed
wording might seem slightly ambiguous for the REINDEX DATABASE case.
It might be clearer to say something like the following:

Reindexing a single index or table requires being the owner of
that index of table.  REINDEX DATABASE and REINDEX SYSTEM
require being the owner of the database, and REINDEX SCHEMA
requires being the owner of the schema (note that the user can
therefore rebuild indexes of tables owned by other users).
Reindexing a shared catalog requires being the owner of the
shared catalog, even if the user is the owner of the specified
database or schema.  Of course, superusers can always reindex
anything.

Nathan

Re: [PATCH] Improve geometric types

[Tom Lane]

Tomas Vondra  writes:
> On 07/29/2018 05:14 PM, Tomas Vondra wrote:
>> FWIW I think this should fix it. Can someone with access to an affected
>> machine confirm?

> Gah, shouldn't have posted before trying to compile it. Here is a fixed
> version of the fix.

Sure, I'll try this on prairiedog.  It's slow though ...

regards, tom lane

Fwd: Would like to help with documentation for Postgres 11

[Michael Goldshteyn]

I sent the below email to pgsql-committers, but I suppose that
pgsql-hackers list might have been the more appropriate mailing list.

-- Forwarded message -
From: Michael Goldshteyn 
Date: Sun, Jul 29, 2018 at 11:49 AM
Subject: Would like to help with documentation for Postgres 11
To: 


I would like to offer some help writing and improving the English
documentation for some of the new features and changes in Postgres 11. If I
can get an email of where such help would be appreciated, so I can choose a
feature I am familiar with, I would be glad to help.

Thanks,

Michael Goldshteyn

Re: [PATCH] Improve geometric types

[Tom Lane]

I wrote:
> Tomas Vondra  writes:
>> On 07/29/2018 05:14 PM, Tomas Vondra wrote:
>>> FWIW I think this should fix it. Can someone with access to an affected
>>> machine confirm?

>> Gah, shouldn't have posted before trying to compile it. Here is a fixed
>> version of the fix.

> Sure, I'll try this on prairiedog.  It's slow though ...

Yup, this fixes the core regression tests on that machine.
I was too lazy to try contrib.

regards, tom lane

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 08:02 PM, Tom Lane wrote:
> I wrote:
>> Tomas Vondra  writes:
>>> On 07/29/2018 05:14 PM, Tomas Vondra wrote:
 FWIW I think this should fix it. Can someone with access to an affected
 machine confirm?
> 
>>> Gah, shouldn't have posted before trying to compile it. Here is a fixed
>>> version of the fix.
> 
>> Sure, I'll try this on prairiedog.  It's slow though ...
> 
> Yup, this fixes the core regression tests on that machine.
> I was too lazy to try contrib.
> 

OK, thanks for confirming. I'll get it committed and we'll see what the
animals think soon.

regards

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

Re: Tips on committing

[Peter Geoghegan]

On Wed, Jul 11, 2018 at 4:35 PM, Peter Geoghegan  wrote:
> You've convinced me that we should definitely have such a list. I've
> put it on my TODO list.

I started this Wiki page:

https://wiki.postgresql.org/wiki/Committing_checklist

I've tried to avoid being too prescriptive. This is a work in progress.

-- 
Peter Geoghegan

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 05:11 PM, Tomas Vondra wrote:
> 
> 
> On 07/29/2018 04:31 PM, Jeff Janes wrote:
>>
>>
>> On Sat, Jul 28, 2018 at 9:54 PM, Tomas Vondra
>> mailto:tomas.von...@2ndquadrant.com>> wrote:
>>
>>
>>
>> I've committed the first two parts, after a review and testing.
>>
>>
>> I'm getting a compiler warning now:
>>
>> geo_ops.c: In function 'line_closept_point':
>> geo_ops.c:2528:7: warning: variable 'retval' set but not used
>> [-Wunused-but-set-variable]
>>   bool  retval;
>>  
> 
> Yeah, the variable is apparently only used in an assert. Will fix.
> 

This should fix it I guess, and it's how we deal with unused return
values elsewhere. I've considered using USE_ASSERT_CHECKING here, but it
seems rather ugly with that. I'll wait for Emre's opinion ...

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 621b5c33ef..7e53ab819c 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -2535,7 +2535,11 @@ line_closept_point(Point *result, LINE *line, Point *point)
 	/* We drop a perpendicular to find the intersection point. */
 	line_construct(&tmp, point, line_invsl(line));
 	retval = line_interpt_line(&closept, line, &tmp);
-	Assert(retval);		/* XXX: We need something better. */
+
+	/* For perpendicular lines, the intersection should exist. */
+	Assert(retval);
+
+	(void) retval;	/* silence compiler warning in non-assert builds */
 
 	if (result != NULL)
 		*result = closept;

Re: [PATCH] Improve geometric types

[Tom Lane]

Tomas Vondra  writes:
> This should fix it I guess, and it's how we deal with unused return
> values elsewhere. I've considered using USE_ASSERT_CHECKING here, but it
> seems rather ugly with that. I'll wait for Emre's opinion ...

I think what you want is to mark the variable with
PG_USED_FOR_ASSERTS_ONLY.

regards, tom lane

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 07/29/2018 10:57 PM, Tom Lane wrote:
> Tomas Vondra  writes:
>> This should fix it I guess, and it's how we deal with unused return
>> values elsewhere. I've considered using USE_ASSERT_CHECKING here, but it
>> seems rather ugly with that. I'll wait for Emre's opinion ...
> 
> I think what you want is to mark the variable with
> PG_USED_FOR_ASSERTS_ONLY.
> 

Oh, good idea. I don't think I've ever used that macro and I've
completely forgotten about it. Committed that way.


regards

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

Re: Bizarre behavior in libpq's searching of ~/.pgpass

[Tom Lane]

I wrote
> I noticed that there's some strange coding in libpq's choice of
> what hostname to use for searching ~/.pgpass for a password.
> ...

> So my first thought was that we should go back to the pre-v10 behavior
> of considering only the host parameter, which it looks like would only
> require removing the "if" bit above.

> But on second thought, I'm not clear that the pre-v10 behavior is really
> all that sane either.  What it means is that if you specify only hostaddr,
> the code will happily grab your localhost password and send it off to
> whatever server hostaddr references.  This is unlikely to be helpful,
> and it could even be painted as a security breach --- the remote server
> could ask for your password in plaintext and then capture it.

> What seems like a saner definition is "use host if it's specified
> (nonempty), else use hostaddr if it's specified (nonempty), else
> fall back to localhost".  That avoids sending a password somewhere
> it doesn't belong, and allows a useful ~/.pgpass lookup in cases
> where only hostaddr is given -- you just need to make an entry
> with the numeric IP address in the host column.

> I think it's not too late to make v11 work that way, but I wonder
> what we ought to do in v10.  Comments?

Here's a proposed patch to adopt that behavior.  I'm still of mixed
mind whether to push this into v10 ... but we definitely need some
change in v10, because it's not acting as per its docs.

regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index d67212b..0bcaeb9 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 1020,1026 
  
  Note that authentication is likely to fail if host
  is not the name of the server at network address hostaddr.
! Also, note that host rather than hostaddr
  is used to identify the connection in a password file (see
  ).
 
--- 1020,1027 
  
  Note that authentication is likely to fail if host
  is not the name of the server at network address hostaddr.
! Also, when both host and hostaddr
! are specified, host
  is used to identify the connection in a password file (see
  ).
 
*** myEventProc(PGEventId evtId, void *evtIn
*** 7521,7533 
 used.  (Therefore, put more-specific entries first when you are using
 wildcards.) If an entry needs to contain : or
 \, escape this character with \.
!A host name of localhost matches both TCP (host name
!localhost) and Unix domain socket (pghost empty
!or the default socket directory) connections coming from the local
!machine. In a standby server, a database name of replication
 matches streaming replication connections made to the master server.
!The database field is of limited usefulness because
!users have the same password for all databases in the same cluster.

  

--- 7522,7538 
 used.  (Therefore, put more-specific entries first when you are using
 wildcards.) If an entry needs to contain : or
 \, escape this character with \.
!The host name field is matched to the host connection
!parameter if that is specified, otherwise to
!the hostaddr parameter if that is specified; if neither
!are given then the host name localhost is searched for.
!A host name field of localhost will also match
!Unix-domain socket connections when the host parameter
!equals the installation's default socket directory path.
!In a standby server, a database name of replication
 matches streaming replication connections made to the master server.
!The database field is of limited usefulness otherwise,
!because users have the same password for all databases in the same cluster.

  

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index bd7dac1..c4d27c4 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** connectOptions2(PGconn *conn)
*** 1065,1072 
  	}
  
  	/*
! 	 * Supply default password if none given.  Note that the password might be
! 	 * different for each host/port pair.
  	 */
  	if (conn->pgpass == NULL || conn->pgpass[0] == '\0')
  	{
--- 1065,1072 
  	}
  
  	/*
! 	 * If password was not given, try to look it up in password file.  Note
! 	 * that the result might be different for each host/port pair.
  	 */
  	if (conn->pgpass == NULL || conn->pgpass[0] == '\0')
  	{
*** connectOptions2(PGconn *conn)
*** 1094,1108 
  			for (i = 0; i < conn->nconnhost; i++)
  			{
  /*
!  * Try to get a password for this host from pgpassfile. We use
!  * host name rather than host address in the same manner as
!  * PQhost().
   */
! char	   *pwhost = conn->connhost[i].host;
  
! if (conn->connhost[i].type == CHT_HOS

Re: Fwd: Would like to help with documentation for Postgres 11

[Justin Pryzby]

On Sun, Jul 29, 2018 at 11:50:40AM -0500, Michael Goldshteyn wrote:
> I would like to offer some help writing and improving the English
> documentation for some of the new features and changes in Postgres 11. If I
> can get an email of where such help would be appreciated, so I can choose a
> feature I am familiar with, I would be glad to help.

The documentation is expected to be commited with the feature, so what's
currently in place is expected to be adequate and accuate.

You could review any documentation changes since v10.  Any issues or
improvements you can discuss or send patch to -hackers.

Maybe something like:
git log -p REL_10_4..REL_11_BETA2 doc/
..or just git diff which would not show commits separately but also would not
show multiple updates for same feature (like: commit: feature... commit:
improve documentation... commit: fix typos... commit: change default)

Alternately you can look at the pg11 release notes (or git log?) for list of
features and check that corresponding documentation exists and is accurate.
That could conceivably expose implementation bugs, eg. with edge cases.
git log REL_10_4..REL_11_BETA2

Justin

Re: Fwd: Would like to help with documentation for Postgres 11

[Tom Lane]

Justin Pryzby  writes:
> On Sun, Jul 29, 2018 at 11:50:40AM -0500, Michael Goldshteyn wrote:
>> I would like to offer some help writing and improving the English
>> documentation for some of the new features and changes in Postgres 11. If I
>> can get an email of where such help would be appreciated, so I can choose a
>> feature I am familiar with, I would be glad to help.

> The documentation is expected to be commited with the feature, so what's
> currently in place is expected to be adequate and accuate.

There is, however, a fair amount of stuff that was written by people whose
first language isn't English, and it shows.  So plain old copy-editing
is surely welcome, if you have a mind to do that.

regards, tom lane

Re: Bizarre behavior in libpq's searching of ~/.pgpass

[Tomas Vondra]

On 07/29/2018 11:15 PM, Tom Lane wrote:
> Here's a proposed patch to adopt that behavior.  I'm still of mixed
> mind whether to push this into v10 ... but we definitely need some
> change in v10, because it's not acting as per its docs.

Is there actually a useful use case working in v10 and broken by your
proposed patch? I can't think of any, and even if there is one it's
likely no one is aware of it as the docs were not updated. Considering
this could also be painted as a security issue (people generally don't
connect from servers to random machines, but still ...), I'd vote for
pushing this to v10.

regards

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

Re: [HACKERS] Parallel Append implementation

[Thomas Munro]

On Thu, May 10, 2018 at 7:08 AM, Robert Haas  wrote:
>  [parallel-append-doc-v2.patch]

+plans just as they can in any other plan.  However, in a parallel plan,
+it is also possible that the planner may choose to substitute a
+Parallel Append node.

Maybe drop "it is also possible that "?  It seems a bit unnecessary
and sounds a bit odd followed by "may ", but maybe it's just me.

+Also, unlike a regular Append node, which can only have
+partial children when used within a parallel plan, Parallel
+Append node can have both partial and non-partial child plans.

Missing "a" before "Parallel".

+Non-partial children will be scanned by only a single worker, since

Are we using "worker" in a more general sense that possibly includes
the leader?  Hmm, yes, other text on this page does that too.  Ho hum.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Add SKIP LOCKED to VACUUM and ANALYZE

[Bossart, Nathan]

On 7/24/18, 8:07 PM, "Michael Paquier"  wrote:
> Hm...  I have not imagined this part but adding a new layer is sort of
> ugly, and an extra one would be needed for CLUSTER as well, in which
> case adding cluster-related logs into vacuum.c introduces a circular
> dependency with cluster.c.  What about just skipping this refactoring
> and move to the point where CLUSTER also gains its log queries directly
> in cluster_rel?  VACUUM FULL is also not going to run for autovacuum, so
> that's less confusion with IsAutoVacuumWorkerProcess().

Since vacuum_rel() already obtains an AccessExclusiveLock on the
relation for VACUUM FULL, we might be able to skip altering
cluster_rel().  I think we will need to alter it if we are going to
add SKIP LOCKED to CLUSTER, though.

Nathan

Re: Covering GiST indexes

[Thomas Munro]

On Sun, Jul 29, 2018 at 10:50 PM, Andrey Borodin  wrote:
> Thanks! The problem appeared with commit 701fd0b [0] which dropped
> validation rules checked in failed test.  Here's the patch with fixed tests.

Thanks.  I received the attachment.

Just as an FYI, something about the way your mail client encoded it
prevented the mail archives from picking it up (according to someone
who knows much more about these things than me, it's buried in the
"multipart/mixed" part, so not visible to the mail archive if it
extracts only the "text/plain" part, or something like that).  That
didn't happen on any of your other patches.  I have no opinion of
whether that's a bug in the mail archive or an unhelpful mail client
or a non-ideal way to post patches, but you can see here that we don't
have the patch in the usual place:

https://www.postgresql.org/message-id/850AE105-F89A-42E4-AD40-FBC6EA5A5A00%40yandex-team.ru

That explains why cfbot didn't automatically test your new patch, so
it still show as broken here:
http://cfbot.cputube.org/andrey-borodin.html

-- 
Thomas Munro
http://www.enterprisedb.com

Adding a note to protocol.sgml regarding CopyData

[Tatsuo Ishii]

In libpq.sgml following is stated:

Before PostgreSQL protocol 3.0, it was 
necessary
for the application to explicitly send the two characters
\. as a final line to indicate to the server that it 
had
finished sending COPY data.  While this still works, 
it is deprecated and the
special meaning of \. can be expected to be removed 
in a
future release.  It is sufficient to call 
PQendcopy after
having sent the actual data.

I think this should be mentioned in protocol.sgml as well. Developers
who wish to develop programs that understand frontend/backend protocol
should be able to focus on protocol.sgml. Attached is a patch for
this.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jpdiff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 46d7e19..a98e4af 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1154,6 +1154,20 @@ SELCT 1/0;
 (if successful) or ErrorResponse (if not).

 
+   
+ 
+   Before PostgreSQL protocol 3.0, it was necessary
+   for the application to explicitly send the two characters
+   \. as a final line to indicate to the server that it
+   had finished sending COPY data.  Programs
+   implementing COPY in protocol 3.0
+   including PostgreSQL need to check and
+   ignore
+   \. just before COPYDone message for backward
+   compatibility sake. This requirement may be removed in the future.
+ 
+   
+

 In the event of a backend-detected error during copy-in mode (including
 receipt of a CopyFail message), the backend will issue an ErrorResponse

Re: Would like to help with documentation for Postgres 11

[Tatsuo Ishii]

> Justin Pryzby  writes:
>> On Sun, Jul 29, 2018 at 11:50:40AM -0500, Michael Goldshteyn wrote:
>>> I would like to offer some help writing and improving the English
>>> documentation for some of the new features and changes in Postgres 11. If I
>>> can get an email of where such help would be appreciated, so I can choose a
>>> feature I am familiar with, I would be glad to help.
> 
>> The documentation is expected to be commited with the feature, so what's
>> currently in place is expected to be adequate and accuate.
> 
> There is, however, a fair amount of stuff that was written by people whose
> first language isn't English, and it shows.  So plain old copy-editing
> is surely welcome, if you have a mind to do that.

BTW, the CommitFest application seems not to pick up threads other
than in pgsql-hackers. This is usually fine. However if people want to
contribute document *only* patches and send it to pgsql-docs then
tries to register it into CF, the CF app does not recognize it. I am
not sure if this is intended behavior or not.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

[Michael Paquier]

On Sun, Jul 29, 2018 at 04:11:38PM +, Bossart, Nathan wrote:
> On 7/27/18, 7:10 PM, "Michael Paquier"  wrote:
> > No problem.  If there are no objections, I am going to fix the REINDEX
> > issue first and back-patch.  Its patch is the least invasive of the
> > set.
> 
> This seems like a reasonable place to start.  I took a closer look at
> 0003.

Thanks for looking at it!

> This is added to ReindexMultipleTables(), which is used for REINDEX
> SCHEMA, REINDEX SYSTEM, and REINDEX DATABASE.  Presently, REINDEX
> SCHEMA requires being the owner of the schema, and REINDEX SYSTEM and
> REINDEX DATABASE require being the owner of the database.  So, if a
> role is an owner of a database or the pg_catalog schema, they are able
> to reindex shared catalogs like pg_authid.

Yeah, I was testing that yesterday night and bumped on this case when
trying do a REINDEX SCHEMA pg_class.  The point is that you can simplify
the check and remove pg_database_ownercheck as there is already an ACL
check on the database/system/schema at the top of the routine, so you
are already sure that pg_database_ownercheck() or
pg_namespace_ownercheck would return true.  This shaves a few cycles as
well for each relation checked.

> I also noticed that this patch causes shared relations to be skipped
> silently.  Perhaps we should emit a WARNING or DEBUG message when this
> happens, at least for REINDEXOPT_VERBOSE.

That's intentional.  I thought about that as well, but I am hesitant to
do so as we don't bother mentioning the other relations skipped.
REINDEX VERBOSE also shows up what are the tables processed, so it is
easy to guess what are the tables skipped, still more painful.  And the
documentation changes added cover the gap.

> I noticed that there is no mention that the owner of a schema can do
> REINDEX SCHEMA, which seems important to note.  Also, the proposed
> wording might seem slightly ambiguous for the REINDEX DATABASE case.
> It might be clearer to say something like the following:
> 
> Reindexing a single index or table requires being the owner of
> that index of table.  REINDEX DATABASE and REINDEX SYSTEM
> require being the owner of the database, and REINDEX SCHEMA
> requires being the owner of the schema (note that the user can
> therefore rebuild indexes of tables owned by other users).
> Reindexing a shared catalog requires being the owner of the
> shared catalog, even if the user is the owner of the specified
> database or schema.  Of course, superusers can always reindex
> anything.

+1, I have included your suggestions.  The patch attached applies easily
down to 9.5 where REINDEX SCHEMA was added.  For 9.4 and 9.3, there is
no schema case, still the new check is similar.  The commit message is
slightly changed so as there is no mention of REINDEX SCHEMA.  9.3 needs
a slight change compared to 9.4 as well.

So attached are patches for 9.5~master, 9.4 and 9.3 with commit
messages.  Does that look fine to folks of this thread?
--
Michael
From d82e9df826b29bf9030dc97551bdd0bef894677d Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 30 Jul 2018 09:29:32 +0900
Subject: [PATCH] Restrict access to reindex of shared catalogs for
 non-privileged users

A database owner running a database-level REINDEX has the possibility to
also do the operation on shared system catalogs without being an owner
of them, which allows him to block resources it should not have access
to.  For example, PostgreSQL would go unresponsive and even block
authentication if a lock is waited for pg_authid.  This commit makes
sure that a user running a REINDEX SYSTEM or DATABASE only works on the
following relations:
- The user is a superuser
- The user is the table owner
- The user is the database owner, only if the relation worked on is not
shared.

Reported-by: Lloyd Albin, Jeremy Schneider
Author: Michael Paquier
Reviewed by: Nathan Bossart, Kyotaro Horiguchi
Discussion: https://postgr.es/m/152512087100.19803.12733865831237526...@wrigleys.postgresql.org
---
 doc/src/sgml/ref/reindex.sgml|  3 ++-
 src/backend/commands/indexcmds.c | 11 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index a795dfa325..00c024 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -214,7 +214,8 @@ REINDEX { INDEX | TABLE | DATABASE | SYSTEM } nam
Reindexing a single index or table requires being the owner of that
index or table.  Reindexing a database requires being the owner of
the database (note that the owner can therefore rebuild indexes of
-   tables owned by other users).  Of course, superusers can always
+   tables owned by other users).  Reindexing a shared catalog requires
+   being the owner of that shared catalog.  Of course, superusers can always
reindex anything.
   
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/in

Re: adding tab completions

[Justin Pryzby]

Sorry for the delay..this got lost while catching up after being out of town..

On Thu, Jun 28, 2018 at 02:20:39PM +0300, Arthur Zakirov wrote:
> Thank you for the new version.
> 
> On Wed, Jun 27, 2018 at 03:10:51PM -0500, Justin Pryzby wrote:
> > Thanks - I've done this in the attached.  It works well for having minimal
> > logic.
> 
> I think everthing is OK. But I didn't notice in previous version of the
> patch that there is no braces in EXPLAIN and VACUUM conditions. I attached
> diff file to show it.
> 
> Also I included TEXT, XML, JSON, YAML items for FORMAT option in the
> diff file. The patch shows ",", ")", "ON", "OFF" for all options, but
> FORMAT requires format type to be specified. Please consider this change
> only as a suggestion.

Your suggestion is good, so attached updated patch.

> > Actually..another thought: since toast tables may be VACUUM-ed, should I
> > introduce Query_for_list_of_tpmt ?

I didn't include this one yet though.

Feel free to bump to next CF.

Justin
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7bb47ea..1d235a3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -705,6 +705,7 @@ static const SchemaQuery Query_for_list_of_tmf = {
"pg_catalog.pg_class c",
/* selcondition */
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+   CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ", "
CppAsString2(RELKIND_FOREIGN_TABLE) ")",
/* viscondition */
@@ -,6 +2223,7 @@ psql_completion(const char *text, int start, int end)
"fillfactor",
"parallel_workers",
"log_autovacuum_min_duration",
+   "toast_tuple_target",
"toast.autovacuum_enabled",
"toast.autovacuum_freeze_max_age",
"toast.autovacuum_freeze_min_age",
@@ -2703,7 +2705,7 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_LIST2("TABLE", "MATERIALIZED VIEW");
/* Complete PARTITION BY with RANGE ( or LIST ( or ... */
else if (TailMatches2("PARTITION", "BY"))
-   COMPLETE_WITH_LIST2("RANGE (", "LIST (");
+   COMPLETE_WITH_LIST3("RANGE (", "LIST (", "HASH (");
/* If we have xxx PARTITION OF, provide a list of partitioned tables */
else if (TailMatches2("PARTITION", "OF"))

COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables, "");
@@ -2996,14 +2998,27 @@ psql_completion(const char *text, int start, int end)
else if (Matches1("EXECUTE"))
COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
 
-/* EXPLAIN */
-
-   /*
-* Complete EXPLAIN [ANALYZE] [VERBOSE] with list of EXPLAIN-able 
commands
-*/
+/*
+ * EXPLAIN [ ( option [, ...] ) ] statement
+ * EXPLAIN [ ANALYZE ] [ VERBOSE ] statement
+ */
else if (Matches1("EXPLAIN"))
-   COMPLETE_WITH_LIST7("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE",
-   "ANALYZE", "VERBOSE");
+   COMPLETE_WITH_LIST8("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE",
+   "ANALYZE", "VERBOSE", 
"(");
+   else if (HeadMatches2("EXPLAIN", "("))
+   {
+   if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
+   COMPLETE_WITH_LIST7("ANALYZE", "VERBOSE", "COSTS", 
"BUFFERS", "TIMING", "SUMMARY", "FORMAT");
+   else if (TailMatches1("FORMAT"))
+   COMPLETE_WITH_LIST4("TEXT", "XML", "JSON", "YAML");
+   else if 
(TailMatches1("ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
+   COMPLETE_WITH_LIST4(",", ")", "ON", "OFF");
+   else
+   COMPLETE_WITH_LIST2(",", ")");
+   }
+   else if (HeadMatches2("EXPLAIN", MatchAny) && ends_with(prev_wd, ')'))
+   /* If the parenthesis are balanced, the list is apparently 
parsed as a single word */
+   COMPLETE_WITH_LIST5("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE");
else if (Matches2("EXPLAIN", "ANALYZE"))
COMPLETE_WITH_LIST6("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE",
"VERBOSE");
@@ -3563,33 +3578,48 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_CONST("OPTIONS");
 
 /*
- * VACUUM [ FULL | FREEZE ] [ VERBOSE ] [ table ]
- * VACUUM [ FULL | FREEZE ] [ VERBOSE ] ANALYZE [ table [ (column [, ...] ) ] ]
+ * VACUUM [ ( option [, ...] ) ] [ table_and_columns [, ...] ]
+ * VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns [, 
...] ]
  */
else if (Matches1("VACUUM"))
-   COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tm,
+   COMPL

Re: Usability fail with psql's \dp command

[Fabien COELHO]

My 0.02¤: this creates an exception for anyone trying to parse the output.
I would have preferred empty logically meaning no rights, and the default
being spelled out explicitely.


Uh, who'd be trying to parse the output of \dp?


Ok. Maybe humans?

Note that 'No privileges' could be somehow interpreted as "default 
privileges" (no "special/given" privileges) or as "no permissions at all", 
so there is still some ambiguity, at least for me.



We could certainly consider the explicit-default approach (and it's one
of the options I suggested), but to my mind we should evaluate the
options entirely on what humans find readable, with exactly zero weight
to machine readability.


Ok. So I agree with your suggestion, on the ground of avoiding a special 
output syntax in one particular case if possible.


Attached is a quick and dirty attempt at regenerating default privileges 
from dp query, with an added join on roles & and test on kind.


I'm not 100% sure of the list of privileges for all types, and I do not 
like much having them in a query like that because in the unlikely event 
that a new one is added, the query output suddenly becomes false.


From a programming point of view there is another pain with that approach 
as "describe.c" uses "printACLColumn", but this version would need the 
kind and the owner role as well, and it seems that all 11 instances of 
printACLColumn should be adapted as well.


As for David point of breaking anything from a user perspective, as the 
current output is currently ambiguous thus unreliable/unusable, I think it 
is more a bug fix than anything else.


--
Fabien.

dp.sql
Description: application/sql

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

[Thomas Munro]

On Thu, May 17, 2018 at 8:20 PM, Paul Guo  wrote:
> Thanks. I tentatively submitted a patch (See the attachment).

Hi Paul,

It looks like you missed a couple of changes in the contrib/btree_gist
bit and varbit tests, so make check-world fails:

- Index Cond: ((a >= B'100'::"bit") AND (a <= B'101'::"bit"))
+ Index Cond: ((a >= '100'::"bit") AND (a <= '101'::"bit"))

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Making "COPY partitioned_table FROM" faster

[Melanie Plageman]

On Sat, Jul 28, 2018 at 6:00 PM, David Rowley 
wrote:

> Oops.  Must've fallen off in transit :) Hopefully, it's more firmly
> attached this time.
>

LGTM. Status changed to "ready for committer"

RE: Recovery performance of standby for multiple concurrent truncates on large tables

[Jamison, Kirk]

Hi Andres,

> I think this is a case where the potential work arounds are complex enough to 
> use significant resources to get right, and are likely to make properly 
> fixing the issue harder. I'm willing to comment on proposals that claim not 
> to be problmatic in those regards, but I have *SERIOUS* doubts they exist.

Alright. But I'd still try and ask your thoughts about it (below).
The proposed design touches the buffer invalidation during recovery process of 
standby server.

The question was about "how" to remember those buffers that contain 
truncate/drop tables, right?

1. Because the multiple scans of the whole shared buffer per concurrent 
truncate/drop table was the cause of the time-consuming behavior, DURING the 
failover process while WAL is being applied, we temporary delay the scanning 
and invalidating of shared buffers. At the same time, we remember the 
relations/relfilenodes (of dropped/truncated tables) by adding them in a hash 
table called "skip list". 
2. After WAL is applied, the checkpoint(or bg writer) scans the shared buffer 
only ONCE, compare the pages against the skip list, and invalidates the 
relevant pages. After deleting the relevant pages on the shared memory, it will 
not be written back to the disk.

Assuming the theory works, this design will only affect the behavior of 
checkpointer (or maybe bg writer) during recovery process / failover. Any 
feedback, thoughts?

BTW, are there any updates whether the community will push through anytime soon 
regarding the buffer mapping implementation you mentioned?


Regards,
Kirk Jamison

Re: Usability fail with psql's \dp command

[Kyotaro HORIGUCHI]

Hello.

At Sun, 29 Jul 2018 21:34:29 -0400 (EDT), Fabien COELHO  
wrote in 
> 
> >> My 0.02¤: this creates an exception for anyone trying to parse the
> >> output.
> >> I would have preferred empty logically meaning no rights, and the
> >> default
> >> being spelled out explicitely.
> >
> > Uh, who'd be trying to parse the output of \dp?
> 
> Ok. Maybe humans?
> 
> Note that 'No privileges' could be somehow interpreted as "default
> privileges" (no "special/given" privileges) or as "no permissions at
> all", so there is still some ambiguity, at least for me.

FWIW "No privileges" seems to me as "The user cannot access it at
all" with no ambiguity.

Currently the behavior is documented here. (This needs to be
edited.)

https://www.postgresql.org/docs/10/static/sql-grant.html

| If the “Access privileges” column is empty for a given object,
| it means the object has default privileges (that is, its
| privileges column is null). Default privileges always include all
| privileges for the owner, and can include some privileges for
| PUBLIC depending on the object type, as explained above. The
| first GRANT or REVOKE on an object will instantiate the default
| privileges (producing, for example, {miriam=arwdDxt/miriam}) and
| then modify them per the specified request.

So it changes the existing documented behavior.

What is most significant to me here is it's confusing that the
empty representation means rather opposite things for
pg_class.relacl and the correspondent in \dp's output.

  relacl | Access privileges
 +--
  (null) | joe=arwdDxt/joe
  {} | (null)

> > We could certainly consider the explicit-default approach (and it's
> > one
> > of the options I suggested), but to my mind we should evaluate the
> > options entirely on what humans find readable, with exactly zero
> > weight
> > to machine readability.
> 
> Ok. So I agree with your suggestion, on the ground of avoiding a
> special output syntax in one particular case if possible.

\dp is a convenient shortcut for users so the output should be
intuitive or easy-to-grasp. If we wanted to use the output as a
input of other programs, we are to use bare tables.. maybe, or
should handle the special indications.  But, if we were to change
the documented behavior, I'd propose the following.

  relacl | Access privileges
 +--
  (null) | '(default)'
  {} | '(no privilege)'

The parentheses ('()') can be '<>' as pg_stat_activity uses for a
content with a special meaning. ().  Also
I found that \dC shows '(binary coercible)' when the cast is a
relabel. So I'm not confident on whether to use but I'd like to
choose '()' for them. Both are not "parsable" for... a human?

> Attached is a quick and dirty attempt at regenerating default
> privileges from dp query, with an added join on roles & and test on
> kind.
> 
> I'm not 100% sure of the list of privileges for all types, and I do
> not like much having them in a query like that because in the unlikely
> event that a new one is added, the query output suddenly becomes
> false.
> 
> From a programming point of view there is another pain with that
> approach as "describe.c" uses "printACLColumn", but this version would
> need the kind and the owner role as well, and it seems that all 11
> instances of printACLColumn should be adapted as well.
> 
> As for David point of breaking anything from a user perspective, as
> the current output is currently ambiguous thus unreliable/unusable, I
> think it is more a bug fix than anything else.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Covering GiST indexes

[Andrey Borodin]

Thanks, Thomas!

> 30 июля 2018 г., в 3:58, Thomas Munro  
> написал(а):
> 
> On Sun, Jul 29, 2018 at 10:50 PM, Andrey Borodin  wrote:
>> Thanks! The problem appeared with commit 701fd0b [0] which dropped
>> validation rules checked in failed test.  Here's the patch with fixed tests.
> 
> Thanks.  I received the attachment.
> 
> Just as an FYI, something about the way your mail client encoded it
> prevented the mail archives from picking it up 

I'll try to investigate this.

Here's patch one more: another attempt to put it into archives.

Best regards, Andrey Borodin.


0001-Covering-GiST-v4.patch
Description: Binary data

RE: How can we submit code patches that implement our (pending) patents?

[Tsunakawa, Takayuki]

From: Chris Travers [mailto:chris.trav...@adjust.com]
> For example a competitor of yours could copy the relevant pieces of the
> PostgreSQL code, refactor this into a library, and then use it as a
> derivative work and this would be entirely within the copyright license.
> They could then license that library out, and you could not use your patents
> or copyrights to stop them.  As such, a patent license would probably be
> very similar from a business perspective to a global public grant even though
> the two strike me as something which might not offer protection in the case
> of a clean-room implementation.
> 
> I certainly wouldn't be opposed to accepting patents where a license was
> granted to the PostgreSQL Global Developer Group and the community as a
> whole to make full use of the patents in any way covered by the copyright
> license of PostgreSQL (i.e where any use that involves utilizing the
> copyright license for PostgreSQL extends to the patents in question).  But
> I am not sure that a business case would be able to be made for releasing
> any patents under such a license since it means that for anyone else, using
> the patents even in commercial software becomes trivial and enforcement
> would become very difficult indeed.

It looks like you are right in that we can't restrict the use of patents in 
PostgreSQL code to PostgreSQL-based software...  Re-reading Apache License 2.0, 
it doesn't have any restriction either.  But, like Apache License, I think the 
patents can be useful to prevent litigation.


Regards
Takayuki Tsunakawa