Re: [HACKERS] Check for interrupts in bf and xdes crypt()

[Jeff Janes]

On Tue, Dec 15, 2015 at 12:35 AM, Andreas Karlsson  wrote:
> Hi,
>
> Here is a patch which makes it possible to cancel a query which runs the
> crypt() function with the bf or xdes hashing algorithm, e.g. crypt('foo',
> gen_salt('bf', 13)). The md5 algorithm does not run for multiple rounds so
> there is no reason to patch it.
>
> I noticed this problem when I accidentally picked a too high n for the
> number of hash rounds.
>
> I have added a call to CHECK_FOR_INTERRUPTS() after every round, and I could
> not measure any performance hit from this.

Looks good to me.  Applies, builds, passes make check, does what it
says and says what it does.  No need for docs, no noticeable
performance impact.

I've marked it ready for committer.  Also recommend for back-patching.

Cheers,

Jeff


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pam auth - add rhost item

[Grzegorz Sampolski]

Hi there!
I'm alive and working on new patch.
So, I takes into account all suggestions from Tomas and I'll
add additional parameter `usedns' with `yes/no' values to pass
resolved hostname or ip address through rhost_item.

On 12/24/2015 03:35 AM, Michael Paquier wrote:
> On Wed, Dec 16, 2015 at 2:53 AM, Tomas Vondra
>  wrote:
>> Actually, one more thing - the patch should probably update the docs too,
>> because client-auth.sgml currently says this in the "auth-pam" section:
>>
>>
>> ...
>> PAM is used only to validate user name/password pairs.
>> ...
>>
>>
>> I believe that's no longer true, because the patch adds PAM_RHOST to the
>> user/password fields.
>>
>> Regarding the other PAM_* fields, none of them strikes me as very useful for
>> our use case.
>>
>> In a broader sense, I think this patch is quite desirable, despite being
>> rather simple (which is good). I certainly don't agree with suggestions that
>> we can already do things like this through pg_hba.conf. If we're providing
>> PAM authentication, let's make it as complete/useful as possible. In some
>> cases modifying PAM may not be feasible - e.g. some management systems rely
>> on PAM as much as possible, and doing changes in other ways is a major
>> hassle.
> There is no input from the author for more than 1 month, I have marked
> the patch as returned with feedback because of a lack of activity.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Check for interrupts in bf and xdes crypt()

[Alvaro Herrera]

Jeff Janes wrote:
> On Tue, Dec 15, 2015 at 12:35 AM, Andreas Karlsson  wrote:

> > Here is a patch which makes it possible to cancel a query which runs the
> > crypt() function with the bf or xdes hashing algorithm, e.g. crypt('foo',
> > gen_salt('bf', 13)). The md5 algorithm does not run for multiple rounds so
> > there is no reason to patch it.

> Looks good to me.  Applies, builds, passes make check, does what it
> says and says what it does.  No need for docs, no noticeable
> performance impact.
> 
> I've marked it ready for committer.  Also recommend for back-patching.

Pushed, thanks.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Avoid endless futile table locks in vacuuming.

[Tom Lane]

Jeff Janes  writes:
> If a partially-active table develops a slug of stable all-visible,
> non-empty pages at the end of it, then every autovacuum of that table
> will skip the end pages on the forward scan, think they might be
> truncatable, and take the access exclusive lock to do the truncation.
> And then immediately fail when it sees the last page is not empty.
> This can persist for an indefinite number of autovac rounds.

> This is not generally a problem, as the lock is taken conditionally.
> However, the lock is also logged and passed over to any hot standbys,
> where it must be replayed unconditionally.  This can cause query
> cancellations.

> The simple solution is to always scan the last page of a table, so it
> can be noticed that it is not empty and avoid the truncation attempt.

This seems like a reasonable proposal, but I find your implementation
unconvincing: there are two places in lazy_scan_heap() that pay attention
to scan_all, and you touched only one of them.  Thus, if we fail to
acquire cleanup lock on the table's last page on the first try, the
change is for naught.  Shouldn't we be insisting on getting that lock?
Or, if you intentionally didn't change that because it seems like too
high a price to pay, why doesn't the comment reflect that?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Avoid endless futile table locks in vacuuming.

[Jeff Janes]

If a partially-active table develops a slug of stable all-visible,
non-empty pages at the end of it, then every autovacuum of that table
will skip the end pages on the forward scan, think they might be
truncatable, and take the access exclusive lock to do the truncation.
And then immediately fail when it sees the last page is not empty.
This can persist for an indefinite number of autovac rounds.

This is not generally a problem, as the lock is taken conditionally.
However, the lock is also logged and passed over to any hot standbys,
where it must be replayed unconditionally.  This can cause query
cancellations.

The simple solution is to always scan the last page of a table, so it
can be noticed that it is not empty and avoid the truncation attempt.

We could add logic like doing this scan only if wal_level is
hot_standby or higher, or reproducing the REL_TRUNCATE_FRACTION logic
here to scan the last page only if truncation is eminent.  But those
seem like needless complications to try to avoid sometimes scanning
one block.

Cheers,

Jeff
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index 2429889..abc2e28
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 490,495 
--- 490,501 
  	 * relfrozenxid, so we only want to do it if we can skip a goodly number
  	 * of pages.
  	 *
+ 	 * We never skip the last page.  This avoids having every vacuum take the 
+ 	 * access exclusive lock on the table to do a truncation which is doomed
+ 	 * to fail in cases where a table has a stable series of all visible pages
+ 	 * at the end.  This is worth avoiding because the access exclusive lock 
+ 	 * must be replayed on any hot standby, where it can be disruptive.
+ 	 *
  	 * Before entering the main loop, establish the invariant that
  	 * next_not_all_visible_block is the next block number >= blkno that's not
  	 * all-visible according to the visibility map, or nblocks if there's no
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 567,573 
  		else
  		{
  			/* Current block is all-visible */
! 			if (skipping_all_visible_blocks && !scan_all)
  continue;
  			all_visible_according_to_vm = true;
  		}
--- 573,579 
  		else
  		{
  			/* Current block is all-visible */
! 			if (skipping_all_visible_blocks && !scan_all && blkno != nblocks-1)
  continue;
  			all_visible_according_to_vm = true;
  		}

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: bloom filter in Hash Joins with batches

[David Rowley]

On 18 December 2015 at 04:34, Tomas Vondra 
wrote:

> I think ultimately we'll need to measure the false positive rate, so that
> we can use it to dynamically disable the bloom filter if it gets
> inefficient. Also maybe put some of that into EXPLAIN ANALYZE.
>

I'm not so convinced that will be a good idea. What if the filter does not
help much to start with, we disable it because of that, then we get some
different probe values later in the scan which the bloom filter would have
helped to eliminate earlier.

Maybe it would be better to, once the filter is built, simply count the
number of 1 bits and only use the filter if there's less than  1
bits compared to the size of the filter in bits. There's functionality in
bms_num_members() to do this, and there's also __builtin_popcount() in
newer version of GCC, which we could have some wrapper around, perhaps.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] Minor additional changes to 9.5's INSERT documentation

[Peter Geoghegan]

Attached is a patch that makes a few additional changes to the INSERT
documentation. These changes are mostly around formatting. I also
point out that unique index inference is mandatory when using unique
indexes and specifying a conflict target (because unique indexes are
not formally constraints).

-- 
Peter Geoghegan
From 63336a0bee877eefaf3ef1ce9e50a39bdbc766a7 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Thu, 24 Dec 2015 13:15:27 -0800
Subject: [PATCH] Minor copy-editing of INSERT documentation

Fix a few minor issues that remained after the major post-commit
overhaul of ON CONFLICT.
---
 doc/src/sgml/ref/insert.sgml | 56 
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index e710cf4..fe000d5 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -300,14 +300,15 @@ INSERT INTO table_name [ AS table_name unique indexes that,
 without regard to order, contain exactly the
 conflict_target-specified
-columns/expressions are inferred (chosen) as arbiter indexes.  If
-an index_predicate is
-specified, it must, as a further requirement for inference,
-satisfy arbiter indexes.  Note that this means a non-partial
-unique index (a unique index without a predicate) will be inferred
-(and thus used by ON CONFLICT) if such an index
-satisfying every other criteria is available.  If an attempt at
-inference is unsuccessful, an error is raised.
+columns/expressions at least once are inferred (chosen) as arbiter
+indexes.  If an index_predicate is specified, it
+must, as a further requirement for inference, satisfy arbiter
+indexes.  Note that this means a non-partial unique index (a
+unique index without a predicate) will be inferred (and thus used
+by ON CONFLICT) if such an index satisfying
+every other criteria is available.  If an attempt at inference is
+unsuccessful, an error is raised.

 

@@ -493,7 +494,10 @@ INSERT INTO table_name [ AS CREATE UNIQUE INDEX ...  CONCURRENTLY
- before dropping the index being replaced.
+ before dropping the constraint being replaced.  Note that unique
+ index inference must be used to choose
+ unique indexes as arbiters, since unique indexes are not formally
+ classified as constraints.
 

 
@@ -627,11 +631,11 @@ INSERT INTO employees_log SELECT *, current_timestamp FROM upd;
 
   
   
-   Insert or update new distributors as appropriate.  Assumes a unique
-   index has been defined that constrains values appearing in the
-   did column.  Note that the special
-   excluded table is used to reference values originally
-   proposed for insertion:
+   Insert or update new distributors as appropriate.  Example assumes
+   a unique index or constraint has been defined that constrains
+   values appearing in the did column.  Note that
+   the special excluded table is used to reference values
+   originally proposed for insertion:
 
 INSERT INTO distributors (did, dname)
 VALUES (5, 'Gizmo Transglobal'), (6, 'Associated Computing, Inc')
@@ -641,9 +645,10 @@ INSERT INTO distributors (did, dname)
   
Insert a distributor, or do nothing for rows proposed for insertion
when an existing, excluded row (a row with a matching constrained
-   column or columns after before row insert triggers fire) exists.
-   Example assumes a unique index has been defined that constrains
-   values appearing in the did column:
+   column or columns after per-row BEFORE INSERT
+   triggers fire) exists.  Example assumes a unique index or
+   constraint has been defined that constrains values appearing in the
+   did column:
 
 INSERT INTO distributors (did, dname) VALUES (7, 'Redline GmbH')
 ON CONFLICT (did) DO NOTHING;
@@ -651,10 +656,11 @@ INSERT INTO distributors (did, dname) VALUES (7, 'Redline GmbH')
   
   
Insert or update new distributors as appropriate.  Example assumes
-   a unique index has been defined that constrains values appearing in
-   the did column.  WHERE clause is
-   used to limit the rows actually updated (any existing row not
-   updated will still be locked, though):
+   a unique index or constraint has been defined that constrains
+   values appearing in the did column.  A
+   WHERE clause is used, limiting which rows are actually
+   updated when the UPDATE action is taken (any
+   existing rows that are not updated are still locked, though):
 
 -- Don't update existing distributors based in a certain ZIP code
 INSERT INTO distributors AS d (did, dname) VALUES (8, 'Anvil Distribution')
@@ -670,11 +676,11 @@ INSERT INTO distributors (did, dname) VALUES (9, 'Antwerp Design')
   
   
Insert new distributor if possible;  otherwise
-   DO NOTHING.  Example assumes a unique index has been
-   defined that constrains values appearing in the
+   DO NOTHING.  Example assumes a 

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

Hi Vinayak,

On 2015/12/25 21:46, Vinayak Pokale wrote:
> Hi,
> Please find attached patch addressing following comments.
> 
>> The relation OID should be reported and not its name. In case of a
>> relation rename that would not be cool for tracking, and most users
>> are surely going to join with other system tables using it.
> The relation OID is reported instead of relation name.
> The following interface function is called at the beginning to report the
> relation OID once.
> void pgstat_report_command_target(Oid relid)
> 
>> Regarding pg_stat_get_vacuum_progress(): I think a backend can simply be
>> skipped if (!has_privs_of_role(GetUserId(), beentry->st_userid)) (cannot
>> put that in plain English, :))
> Updated in the attached patch.
> 
> In the previous patch, ACTIVITY_IS_VACUUM is set unnecessarily for
> VACOPT_FULL and they are not covered by lazy_scan_heap().
> I have updated it in attached patch and also renamed ACTIVITY_IS_VACUUM to
> COMMAND_LAZY_VACUUM.
> 
> Added documentation for view.
> Some more comments need to be addressed.

I suspect you need to create a new CF entry for this patch in CF 2016-01.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] MergeAttributes type (mod) conflict error detail

[Amit Langote]

On 2015/12/27 3:11, Tom Lane wrote:
> I wrote:
>> Amit Langote  writes:
>>> Any specific reason why it doesn't spell out typmods in the above detail
>>> message?
> 
>> * There's a rough policy in the parser to prefer TypeNameToString
>> when complaining about a TypeName input, rather than reconstructing
>> the type name from the OID.  The reason for this is that we'd rather
>> complain about the type name as entered, not the canonical type name
>> --- for example, if the user typed "float8" it might be a bit confusing
>> if the parser then complains about "double precision".
>>
>> I'm not entirely sure though that that argument should be applied
>> to this particular case, because the other type referred to will
>> certainly get displayed in canonical form.
> 
> On reflection, I think trying to spell both types according to the same
> rules will be the least confusing behavior here.
> 
>> So we could either apply your patch as written, or we could replace
>> only the format_type_be calls with format_type_with_typemod, and
>> then fix TypeNameToString so that it will show the typmod if any.
>> (We'd need to consider whether that behavior is OK for all callers.)
>>
>> Even if we decide this particular case is best handled by presenting
>> canonical type names on both sides, maybe it would be wise to look
>> into whether TypeNameToString should be changed for other callers.
> 
> I looked through the other call sites for TypeNameToString and
> TypeNameListToString.  In none of them does it seem useful to include any
> typmod info in the printout, and in many of them it would be positively
> misleading (e.g., functions do not care about typmod decorations on their
> argument types).  So we should not change the behavior of those functions.
> Perhaps down the road there'll be a use for "TypeNameAndTypmodToString",
> but I don't feel a need for it today.
> 
> So I am thinking your patch is good as proposed, ie, let's use
> format_type_with_typemod here.

I agree. Thanks for adding the tests.

Regards,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/12/23 2:47, Robert Haas wrote:

On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
 wrote:

Moved to next CF because of a lack of reviews.


Thanks, Michael!


I just took a look at this.  I think the basic idea of this patch is
good, but the comments need some work, because they don't really
explain why this should be skipped in the join case.  Maybe something
like this:


Thanks for the review, Robert!


If rel is a base relation, detect whether any system columns were
requested.  (If rel is a join relation, rel->relid will be 0, but
there can be no Var in the target list with relid 0, so we skip this
in that case.) This is a bit of a kluge and might go away someday, so
we intentionally leave it out of the API presented to FDWs.
And the rest as it is currently written.


Agreed.


It might be good, also, to say something about why we never need
fsSystemCol to be true in the joinrel case.


+1 for that.  How about adding something like this:

Note that any such system columns are assumed to be contained in 
fdw_scan_tlist, so we never need fsSystemCol to be true in the joinrel case.


Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2097,2106  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2097,2103 
***
*** 2169,2204  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2166,2213 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var in the target list with relid 0, so we skip
! 	 * this in that case.  Note that any such system columns are assumed to be
! 	 * contained in fdw_scan_tlist, so we never need fsSystemCol to be true in
! 	 * the joinrel case.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel->baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Remove Windows crash dump support?

[Feng Tian]

On Wed, Dec 23, 2015 at 6:14 PM, Craig Ringer  wrote:

> On 22 December 2015 at 23:48, Alex Ignatov 
> wrote:
>
>
>> I think that you can debug crash dump since windbg exists.
>>
>
> Nobody in their right mind uses windbg though. Visual Studio is really
> where it's at and the Express versions make it much more practical.
>

Just FYI.  Most developers of MS server (OS/SQL Server) team use windbg.
Windbg was my only debugger when I worked there and I never used Visual
Studio.




>
> You can't even install Debugging Tools for Windows and Windbg standalone
> anymore.
>
>
>> Also I think that Postgres on Windows number  of instalations is so tiny
>> because people even today think that it is not so solid as unix version
>> thats why you think that nobody use your code ;).
>>
>
> I disagree. Windows Pg users are often at bigger companies and don't talk
> about PostgreSQL as much. Often for fear of reprisals from other database
> vendors they have ongoing relationships with.  At least that's been my
> experience and I'm sure EDB folks will concur.
>
>
>> Today if my memory serves me right this code can not deal with buffer
>> overflow. Am i right?
>>
>
> Stack overflow?
>
> No, it can't. The stack has to be somewhat sane.
>
>
>> May be we need to add this functionality instead of drop support of it
>> entirely
>>
>
> Why? I've never seen any sign anybody has used it, ever.
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] Remove Windows crash dump support?

[Amit Kapila]

On Thu, Dec 24, 2015 at 7:48 AM, Craig Ringer  wrote:

> On 22 December 2015 at 23:46, Tom Lane  wrote:
>
>> > In which version(s) of Windows was this improvement added?
>
>
> Vista and Server 2008 (original, not R2).
>
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181(v=vs.85).aspx
>
> Win7 and Server 2003 are already irrelevant now, and will be absurdly so
> by the time Pg 9.6 comes out.
>
>
Sure, but I think still there could be developers out there or some users
of PostgreSQL who will still be using some such systems.


> The windows feature works better than the in-application crash dump.
>

Have you used this feature of Windows and does it display the call stack
or the information better than what PostgreSQL already has, if you have
any saved dump, that others can also see, that might be useful?
Unfortunately, I am still using Win7, so not able to test using this
new feature.


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