Re: [HACKERS] documentation typo in ALTER TABLE example

[Tatsuo Ishii]

>> Amit Langote, which one was your intention?
> 
> I wanted to show DETACH PARTITION command's usage with a range partitioned
> table (detaching the "oldest" partition).
> 
> So, we should apply Nagata-san's patch.

Thank you for the confirmation. I have pushed the patch.

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


-- 
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] Buildfarm failures on woodlouse (in ecpg-check)

[Christian Ullrich]

* Andrew Dunstan wrote:


On 06/11/2017 11:33 AM, Christian Ullrich wrote:



To build correctly, it requires defining _WIN32_WINNT to be 0x600 or
above (and using an SDK that knows about InitOnceExecuteOnce()).



We already define _WIN32_WINNT to be 0x0600 on all appropriate platforms
(Vista/2008 and above), so I think you could probably just check for
that value.


Not quite; the definition depends on the build toolset, not the build 
platform. When building with VS 2015 and above, _WIN32_WINNT is set to 
0x600 (Vista/2008), while with older compilers, the value is 0x501 
(XP/2003). This is also due to locale issues, but of a different kind, 
and is apparently coincidental.


The build platform does not figure into the target platform, which is 
clearly a good idea for binary distribution reasons.


--
Christian



--
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] documentation typo in ALTER TABLE example

[Amit Langote]

On 2017/06/12 11:01, Tatsuo Ishii wrote:
>> Hi,
>>
>> Attached is a simple patch to fix a documentation typo in
>> the ALTER TABLE example.
> 
> Or the original author's intention might have been something like
> this:
> 
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -1399,7 +1399,7 @@ ALTER TABLE cities
> Detach a partition from partitioned table:
>  
>  ALTER TABLE cities
> -DETACH PARTITION measurement_y2015m12;
> +DETACH PARTITION cities_ab;
> 
> Amit Langote, which one was your intention?

I wanted to show DETACH PARTITION command's usage with a range partitioned
table (detaching the "oldest" partition).

So, we should apply Nagata-san's patch.

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] Getting server crash on Windows when using ICU collation

[Ashutosh Sharma]

PFA patch that fixes the issue described in above thread. As mentioned
in the above thread, the crash is basically happening in varstr_cmp()
function and  it's  only happening on Windows because in varstr_cmp(),
if the collation provider is ICU, we don't even think of calling ICU
functions to compare the string. Infact, we directly attempt to call
the OS function wsccoll*() which is not expected. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Sat, Jun 10, 2017 at 3:25 PM, Ashutosh Sharma  wrote:
> Hi All,
>
> I am seeing a server crash when running queries using ICU collations on
> Windows. Following are the steps to reproduce the crash with the help of
> patch to enable icu feature on Windows - [1],
>
> 1) psql -d postgres
>
> 2) CREATE DATABASE icu_win_test
>  TEMPLATE template0
>  ENCODING 'UTF8'
> LC_CTYPE 'C'
>LC_COLLATE 'C';
>
> 3) \c icu_win_test;
>
> 4) icu_win_test=# select 'B' > 'a' COLLATE "de-u-co-standard-x-icu";
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> The backtrace observed in the core dump is,
>
>> postgres.exe!varstr_cmp(char * arg1, int len1, char * arg2, int len2,
>> unsigned int collid)  Line 1494 C
>   postgres.exe!text_cmp(varlena * arg1, varlena * arg2, unsigned int collid)
> Line 1627 C
>   postgres.exe!text_gt(FunctionCallInfoData * fcinfo)  Line 1738 + 0x12
> bytes C
>   postgres.exe!ExecInterpExpr(ExprState * state, ExprContext * econtext,
> char * isnull)  Line 650 + 0xa bytes C
>   postgres.exe!evaluate_expr(Expr * expr, unsigned int result_type, int
> result_typmod, unsigned int result_collation)  Line 4719 C
>   postgres.exe!evaluate_function(unsigned int funcid, unsigned int
> result_type, int result_typmod, unsigned int result_collid, unsigned int
> input_collid, List * args, char funcvariadic, HeapTupleData * func_tuple,
> eval_const_expressions_context * context)  Line 4272 + 0x50 bytes C
>   postgres.exe!simplify_function(unsigned int funcid, unsigned int
> result_type, int result_typmod, unsigned int result_collid, unsigned int
> input_collid, List * * args_p, char funcvariadic, char process_args, char
> allow_non_const, eval_const_expressions_context * context)  Line 3914 + 0x44
> bytes C
>   postgres.exe!eval_const_expressions_mutator(Node * node,
> eval_const_expressions_context * context)  Line 2627 C
>   postgres.exe!expression_tree_mutator(Node * node, Node * (void)* mutator,
> void * context)  Line 2735 + 0x37 bytes C
>   postgres.exe!expression_tree_mutator(Node * node, Node * (void)* mutator,
> void * context)  Line 2932 + 0x8 bytes C
>   postgres.exe!eval_const_expressions(PlannerInfo * root, Node * node)  Line
> 2413 C
>   postgres.exe!subquery_planner(PlannerGlobal * glob, Query * parse,
> PlannerInfo * parent_root, char hasRecursion, double tuple_fraction)  Line
> 623 + 0x2d bytes C
>
> RCA:
> 
> As seen in the backtrace, the crash is basically happening in varstr_cmp()
> function. AFAICU, this crash is only happening on Windows because in
> varstr_cmp(), if the encoding type is UTF8, we don't even think of calling
> ICU functions to compare the string. Infact, we directly attempt to call the
> OS function wsccoll*().
>
> The point is, if collation provider is ICU, then we should tryto call ICU
> functions for collation support instead of calling OS functions. This thing
> is being taken care inside varstr_cmp() for Linux but surprisingly it's not
> handled for Windows. Please have a look at below code snippet in
> varstr_cmp() to know on how it is being taken care for linux,
>
> #endif   /* WIN32 */
> 
> 
>
> #ifdef USE_ICU
> #ifdef HAVE_UCOL_STRCOLLUTF8
> if (GetDatabaseEncoding() == PG_UTF8)
> {
>UErrorCode status;
>
>status = U_ZERO_ERROR;
>result = ucol_strcollUTF8(mylocale->info.icu.ucol,
>   arg1, len1,
>   arg2, len2,
>  );
>  if (U_FAILURE(status))
>  ereport(ERROR,
>  (errmsg("collation failed: %s", u_errorName(status;
>  }
> else
> #endif
> {
>   int32_t ulen1,
>   ulen2;
>  UChar   *uchar1, *uchar2;
>
> ulen1 = icu_to_uchar(, arg1, len1);
> ulen2 = icu_to_uchar(, arg2, len2);
>
> result = ucol_strcoll(mylocale->info.icu.ucol,
>   uchar1, ulen1,
>   uchar2, ulen2);
>  }
> #else /* not USE_ICU */
> /* shouldn't happen */
>  elog(ERROR, "unsupported collprovider: %c", mylocale->provider);
>  #endif   /* not USE_ICU */
> }
> else
> {
> #ifdef HAVE_LOCALE_T
>  result = strcoll_l(a1p, a2p, mylocale->info.lt);
> #else
> /* shouldn't happen */
>  elog(ERROR, "unsupported collprovider: %c", 

Re: [HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

[vinayak]

Hi,

On 2017/06/10 12:23, Vinayak Pokale wrote:


Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes" > wrote:

>
> Could you please add a "DO CONTINUE" case to one of the test cases? Or
> add a new one? We would need a test case IMO.
>
Yes I will add test case and send updated patch.


I have added new test case for DO CONTINUE.
Please check the attached patch.

Regards,
Vinayak Pokale
NTT Open Source Software Center


WHENEVER-statement-DO-CONTINUE-support.patch
Description: binary/octet-stream

-- 
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] Adding support for Default partition in partitioning

[Ashutosh Bapat]

While the refactoring seems a reasonable way to re-use existing code,
that may change based on the discussion in [1]. Till then please keep
the refactoring patches separate from the main patch. In the final
version, I think the refactoring changes to ATAttachPartition and the
default partition support should be committed separately. So, please
provide three different patches. That also makes review easy.

On Mon, Jun 12, 2017 at 8:29 AM, Jeevan Ladhe
 wrote:
> Hi Ashutosh,
>
> I tried to look into your refactoring code.
> When applied all 3 patches, I got some regression failures, I have fixed all
> of
> them now in attached patches, attached the regression.diffs.
>
> Moving further, I have also made following changes in attached patches:
>
> 1. 0001-Refactor-ATExecAttachPartition.patch
>
> + * There is a case in which we cannot rely on just the result of the
> + * proof
> This comment seems to also exist in current code, and I am not able to
> follow
> which case this refers to. But, IIUC, this comment is for the case where we
> are
> handling the 'key IS NOT NULL' part separately, and if that is the case it
> is
> not needed here in the prologue of the function.
>
> attachPartCanSkipValidation
> +static bool
> +ATCheckValidationSkippable(Relation scanRel, List *partConstraint,
> +  PartitionKey key)
> The function name ATCheckValidationSkippable does not sound very intuitive
> to me,
> and also I think prefix AT is something does not fit here as the function is
> not
> really directly related to alter table command, instead is an auxiliary
> function.
> How about changing it to "attachPartitionRequiresScan" or
> "canSkipPartConstraintValidation"
>
> +   List   *existConstraint = NIL;
> Needs to be moved to inside if block instead.
>
> +   boolskip_validate;
> Needs to be initialized to false, otherwise it can be returned without
> initialization when scanRel_constr is NULL.
>
> +   if (scanRel_constr != NULL)
> instead of this may be we can simply have:
> +   if (scanRel_constr == NULL)
> + return false;
> This can prevent further indentation.
>
> +static void
> +ATValidatePartitionConstraints(List **wqueue, Relation scanRel,
> +  List *partConstraint, Relation rel)
> What about just validatePartitionConstraints()
>
> +   boolskip_validate = false;
> +
> +   /* Check if we can do away with having to scan the table being attached.
> */
> +   skip_validate = ATCheckValidationSkippable(scanRel, partConstraint,
> key);
>
> First assignment is unnecessary here.
>
> Instead of:
> /* Check if we can do away with having to scan the table being attached. */
> skip_validate = ATCheckValidationSkippable(scanRel, partConstraint, key);
>
> /* It's safe to skip the validation scan after all */
> if (skip_validate)
> ereport(INFO,
> (errmsg("partition constraint for table \"%s\" is implied by existing
> constraints",
> RelationGetRelationName(scanRel;
>
> Following change can prevent further indentation:
> if (ATCheckValidationSkippable(scanRel, partConstraint, key))
> {
> ereport(INFO,
> (errmsg("partition constraint for table \"%s\" is implied by existing
> constraints",
> RelationGetRelationName(scanRel;
> return;
> }
> This way variable skip_validate will not be needed.
>
> Apart from this, I see that the patch will need change depending on how the
> fix
> for validating partition constraints in case of embedded NOT-NULL[1] shapes
> up.
>
> 2. 0003-Refactor-default-partitioning-patch-to-re-used-code.patch
>
> + * In case the new partition bound being checked itself is a DEFAULT
> + * bound, this check shouldn't be triggered as there won't already exists
> + * the default partition in such a case.
> I think above comment in DefineRelation() is not applicable, as
> check_default_allows_bound() is called unconditional, and the check for
> existence
> of default partition is now done inside the check_default_allows_bound()
> function.
>
>   * This function checks if there exists a row in the default partition that
>   * fits in the new partition and throws an error if it finds one.
>   */
> Above comment for check_default_allows_bound() needs a change now, may be
> something like this:
>   * This function checks if a default partition already exists and if it
> does
>   * it checks if there exists a row in the default partition that fits in
> the
>   * new partition and throws an error if it finds one.
>   */
>
> List   *new_part_constraints = NIL;
> List   *def_part_constraints = NIL;
> I think above initialization is not needed, as the further assignments are
> unconditional.
>
> + if (OidIsValid(default_oid))
> + {
> + Relation default_rel = heap_open(default_oid, AccessExclusiveLock);
> We already have taken a lock on default and here we should be using a NoLock
> instead.
>
> + def_part_constraints =
> get_default_part_validation_constraint(new_part_constraints);
> exceeds 80 columns.
>
> + 

Re: [HACKERS] RTE_NAMEDTUPLESTORE, enrtuples and comments

[Thomas Munro]

On Sun, Jun 11, 2017 at 11:11 PM, Thomas Munro
 wrote:
> On Sun, Jun 11, 2017 at 6:25 PM, Noah Misch  wrote:
>> This fourth point is not necessarily a defect: I wonder if RangeTblEntry is
>> the right place for enrtuples.  It's a concept regularly seen in planner data
>> structures but not otherwise seen at parse tree level.
>
> I agree that this is strange.  Perhaps
> set_namedtuplestore_size_estimates should instead look up the
> EphemeralNamedRelation by rte->enrname to find its way to
> enr->md.enrtuples, but I'm not sure off the top of my head how it
> should get its hands on the QueryEnvironment required to do that.  I
> will look into this on Monday, but other ideas/clues welcome...

Here's some background:  If you look at the interface changes
introduced by 18ce3a4 you will see that it is now possible for a
QueryEnvironment object to be injected into the parser and executor.
Currently the only use for it is to inject named tuplestores into the
system via SPI_register_relation or SPI_register_trigger_data.  That's
to support SQL standard transition tables, but anyone can now use SPI
to expose data to SQL via an ephemeral named relation in this way.
(In future we could imagine other kinds of objects like table
variables, anonymous functions or streams).

The QueryEnvironment is used by the parser to resolve names and build
RangeTblEntry objects.  The planner doesn't currently need it, because
all the information it needs is in the RangeTblEntry, including the
offending row estimate.  Then the executor needs it to get its hands
on the tuplestores.  So the question is: how can we get it into
costsize.c, so that it can look up the EphermalNamedRelationMetaData
object by name, instead of trafficking statistics through parser data
structures?

Here are a couple of ways forward that I can see:

1.  Figure out how to get the QueryEnvironment through more of these
stack frames (possibly inside other objects), so that
set_namedtuplestore_size_estimates can look up enrtuples by enrname:

  set_namedtuplestore_size_estimates <-- would need QueryEnvironment
  set_namedtuplestore_pathlist
  set_rel_size
  set_base_rel_sizes
  make_one_rel
  query_planner
  grouping_planner
  subquery_planner
  standard_planner
  planner
  pg_plan_query
  pg_plan_queries <-- doesn't receive QueryEnvironment
  BuildCachedPlan <-- receives QueryEnvironment
  GetCachedPlan
  _SPI_execute_plan
  SPI_execute_plan_with_paramlist

2.  Rip the row estimation out for now, use a bogus hard coded
estimate like we do in some other cases, and revisit later.  See
attached (including changes from my previous message).
Unsurprisingly, a query plan changes.

Thoughts?

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


fixes-for-enr-rte-review-v2.patch
Description: Binary data

-- 
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] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I tried to look into your refactoring code.
When applied all 3 patches, I got some regression failures, I have fixed
all of
them now in attached patches, attached the regression.diffs.

Moving further, I have also made following changes in attached patches:

*1. 0001-Refactor-ATExecAttachPartition.patch*

+ * There is a case in which we cannot rely on just the result of the
+ * proof
This comment seems to also exist in current code, and I am not able to
follow
which case this refers to. But, IIUC, this comment is for the case where we
are
handling the 'key IS NOT NULL' part separately, and if that is the case it
is
not needed here in the prologue of the function.

attachPartCanSkipValidation
+static bool
+ATCheckValidationSkippable(Relation scanRel, List *partConstraint,
+  PartitionKey key)
The function name ATCheckValidationSkippable does not sound very intuitive
to me,
and also I think prefix AT is something does not fit here as the function
is not
really directly related to alter table command, instead is an auxiliary
function.
How about changing it to "attachPartitionRequiresScan" or
"canSkipPartConstraintValidation"

+   List   *existConstraint = NIL;
Needs to be moved to inside if block instead.

+   boolskip_validate;
Needs to be initialized to false, otherwise it can be returned without
initialization when scanRel_constr is NULL.

+   if (scanRel_constr != NULL)
instead of this may be we can simply have:
+   if (scanRel_constr == NULL)
+ return false;
This can prevent further indentation.

+static void
+ATValidatePartitionConstraints(List **wqueue, Relation scanRel,
+  List *partConstraint, Relation rel)
What about just validatePartitionConstraints()

+   boolskip_validate = false;
+
+   /* Check if we can do away with having to scan the table being
attached. */
+   skip_validate = ATCheckValidationSkippable(scanRel, partConstraint,
key);

First assignment is unnecessary here.

Instead of:
/* Check if we can do away with having to scan the table being attached. */
skip_validate = ATCheckValidationSkippable(scanRel, partConstraint, key);

/* It's safe to skip the validation scan after all */
if (skip_validate)
ereport(INFO,
(errmsg("partition constraint for table \"%s\" is implied by existing
constraints",
RelationGetRelationName(scanRel;

Following change can prevent further indentation:
if (ATCheckValidationSkippable(scanRel, partConstraint, key))
{
ereport(INFO,
(errmsg("partition constraint for table \"%s\" is implied by existing
constraints",
RelationGetRelationName(scanRel;
return;
}
This way variable skip_validate will not be needed.

Apart from this, I see that the patch will need change depending on how the
fix
for validating partition constraints in case of embedded NOT-NULL[1] shapes
up.

*2. 0003-Refactor-default-partitioning-patch-to-re-used-code.patch*

+ * In case the new partition bound being checked itself is a DEFAULT
+ * bound, this check shouldn't be triggered as there won't already exists
+ * the default partition in such a case.
I think above comment in DefineRelation() is not applicable, as
check_default_allows_bound() is called unconditional, and the check for
existence
of default partition is now done inside the check_default_allows_bound()
function.

  * This function checks if there exists a row in the default partition that
  * fits in the new partition and throws an error if it finds one.
  */
Above comment for check_default_allows_bound() needs a change now, may be
something like this:
  * This function checks if a default partition already exists and if it
does
  * it checks if there exists a row in the default partition that fits in
the
  * new partition and throws an error if it finds one.
  */

List   *new_part_constraints = NIL;
List   *def_part_constraints = NIL;
I think above initialization is not needed, as the further assignments are
unconditional.

+ if (OidIsValid(default_oid))
+ {
+ Relation default_rel = heap_open(default_oid, AccessExclusiveLock);
We already have taken a lock on default and here we should be using a NoLock
instead.

+ def_part_constraints =
get_default_part_validation_constraint(new_part_constraints);
exceeds 80 columns.

+ defPartConstraint =
get_default_part_validation_constraint(partBoundConstraint);
similarly, needs indentation.

+
+List *
+get_default_part_validation_constraint(List *new_part_constraints)
+{
Needs some comment. What about:
/*
 * get_default_part_validation_constraint
 *
 * Given partition constraints, this function returns *would be* default
 * partition constraint.
 */

Apart from this, I tried to address the differences in error shown in case
of
attache and create partition when rows in default partition would violate
the
updated constraints, basically I have taken a flag in AlteredTableInfo to
indicate if the relation being scanned is a default partition or a child of
default partition(which I dint like much, but I don't 

Re: [HACKERS] Transition tables vs ON CONFLICT

[Thomas Munro]

On Fri, Jun 9, 2017 at 4:10 PM, Thomas Munro
 wrote:
> On Wed, Jun 7, 2017 at 10:47 AM, Peter Geoghegan  wrote:
>> I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
>> case -- one for updated tuples, and the other for inserted tuples.
>
> [...]
>
> Here is a patch implementing the above.  It should be applied on top
> of transition-tuples-from-wctes-v2.patch[2].

Here's a new version of patch #3.  It's rebased on top of
transition-tuples-from-wctes-v3.patch.  I also moved a comment for
execReplication.c out of this patch into patch #2, correcting a
mistake in my pancake stacking.

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


transition-tuples-from-on-conflict-v2.patch
Description: Binary data

-- 
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Thomas Munro]

On Mon, Jun 12, 2017 at 9:29 AM, Andrew Gierth
 wrote:
>> "Robert" == Robert Haas  writes:
>
>  Robert> I don't see a reason why MakeTransitionCaptureState needs to
>  Robert> force the tuplestores into TopTransactionContext or make them
>  Robert> owned by TopTransactionResourceOwner.
>
> Nor do I, and I'm pretty sure it's leaking memory wholesale within a
> transaction if you have aborted subxacts.
>
> e.g. a few iterations of this, on a table with an appropriate trigger:
>
> savepoint s1;
> insert into foo
>   select i, case when i < 10 then repeat('a',100)
> else repeat('a',1/(i-10)) end
> from generate_series(1,10) i;
> rollback to s1;
>
> This is a bit contrived of course but it shows that there's missing
> cleanup somewhere, either in the form of poor choice of context or
> missing code in the subxact cleanup.

Right, thanks.  I think the fix for this is to use
CurTransactionContext (for memory cleanup) and
CurTransactionResourceOwner (for temporary file cleanup, in case the
tuplestores spill to disk).  The attached does it that way.

With the previous version of the patch, I could see temporary files
accumulating under base/pgsql_tmp from each aborted subtransaction
when I set work_mem = '64kB' to provoke spillage.  With the attached
version, the happy path cleans up (because ExecEndModifyTable calls
DestroyTransitionCaptureState), and the error path does too (because
the subxact's memory and temporary files are automatically cleaned
up).

>  Robert> I mean, it was like that before, but afterTriggers is a global
>  Robert> variable and, potentially, there could still be a pointer
>  Robert> accessible through it to a tuplestore linked from it even after
>  Robert> the corresponding subtransaction aborted, possibly causing some
>  Robert> cleanup code to trip and fall over.  But that can't be used to
>  Robert> justify this case, because the TransitionCaptureState is only
>  Robert> reached through the PlanState tree; if that goes away, how is
>  Robert> anybody going to accidentally follow a pointer to the
>  Robert> now-absent tuplestore?
>
> For per-row triggers with transition tables, a pointer to the transition
> capture state ends up in the shared-data record in the event queue?

Yes.  But the only way for event queue data to last longer than the
execution of the current statement is if it's a deferred trigger.
Only constraint triggers can be deferred, and constraint triggers are
not allowed to have transition tables.  If we wanted to support that
(for example, to handle set-orient FK checks as has been discussed),
we'd need to change the lifetime of the TransitionCaptureState object
(it would need to outlast ModifyTableState).  I've added a note to
that effect to MakeTransitionCaptureState.

So far there seem to be no objections to the inclusion of the pointer
in the event queue that records which transition tables each trigger
invocation should see...?  The extra memory consumption per chunk is
limited by the number of ModifyTable nodes, so it's not going to be a
memory hog, but I thought someone might not like it on other grounds.

On Sat, Jun 10, 2017 at 7:48 AM, Robert Haas  wrote:
> The structure of AfterTriggerSaveEvent with this patch applied looks
> pretty weird.  IIUC, whenever we enter the block guarded by if
> (row_trigger), then either (a) transition_capture != NULL or (b) each
> of the four "if" statements inside that block will separately decide
> to do nothing.  I think now that you're requiring a transition_capture
> for all instances where transition tuplestores are used, you could
> simplify this code.  Maybe just change the outer "if" statement to if
> (row_trigger) and then Assert(transition_capture != NULL)?

Yeah, that was silly, and also the comment was misleading.  It is
possible for row_trigger to be true and transition_capture to be NULL,
so I did it slightly differently.  Fixed in the attached.

Thanks both for the review.  New version of patch #2 attached.

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


transition-tuples-from-wctes-v3.patch
Description: Binary data

-- 
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] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Thomas Munro]

On Sat, Jun 10, 2017 at 6:08 AM, Robert Haas  wrote:
> I have spent some time now studying this patch.  I might be missing
> something, but to me this appears to be in great shape.  A few minor
> nitpicks:
>
> -if ((event == TRIGGER_EVENT_DELETE &&
> !trigdesc->trig_delete_after_row) ||
> -(event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) 
> ||
> - (event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row))
> +if (trigdesc == NULL ||
> +(event == TRIGGER_EVENT_DELETE &&
> !trigdesc->trig_delete_after_row) ||
> +(event == TRIGGER_EVENT_INSERT &&
> !trigdesc->trig_insert_after_row) ||
> +(event == TRIGGER_EVENT_UPDATE &&
> !trigdesc->trig_update_after_row))
>
> I suspect the whitespace changes will get reverted by pgindent, making
> them pointless.  But that's a trivial issue.

Not just a whitespace change: added "trigdesc == NULL" and put the
existing stuff into parens, necessarily changing indentation level.

> +if (mtstate->mt_transition_capture != NULL)
> +{
> +if (resultRelInfo->ri_TrigDesc &&
> +(resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
> + resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
> +{
> +/*
> + * If there are any BEFORE or INSTEAD triggers on the
> + * partition, we'll have to be ready to convert their result
> + * back to tuplestore format.
> + */
> +
> mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
> +mtstate->mt_transition_capture->tcs_map =
> +mtstate->mt_transition_tupconv_maps[leaf_part_index];
> +}
> +else
> +{
> +/*
> + * Otherwise, just remember the original unconverted tuple, 
> to
> + * avoid a needless round trip conversion.
> + */
> +
> mtstate->mt_transition_capture->tcs_original_insert_tuple = tuple;
> +mtstate->mt_transition_capture->tcs_map = NULL;
> +}
> +}
>
> This chunk of code gets inserted between a comment and the code to
> which that comment refers.  Maybe that's not the best idea.

Fixed.  Similar code existed in copy.c, so fixed there too.

> * Does has_superclass have concurrency issues?  After some
> consideration, I decided that it's probably fine as long as you hold a
> lock on the target relation sufficient to prevent it from concurrently
> becoming an inheritance child - i.e. any lock at all.  The caller is
> CREATE TRIGGER, which certainly does.

I added a comment to make that clear.

> * In AfterTriggerSaveEvent, is it a problem that the large new hunk of
> code ignores trigdesc if transition_capture != NULL?  If I understand
> correctly, the trigdesc will be coming from the leaf relation actually
> being updated, while the transition_capture will be coming from the
> relation named in the query.  Is the transition_capture object
> guaranteed to have all the flags set, or do we also need to include
> the ones from the trigdesc?  This also seems to be fine, because of
> the restriction that row-level triggers with tuplestores can't
> participate in inheritance hierarchies.  We can only need to capture
> the tuples for the relation named in the query, not the leaf
> partitions.

Yeah.  I think that's right.

Note that in this patch I was trying to cater to execReplication.c so
that it wouldn't have to construct a TransitionCaptureState.  It
doesn't actually support hierarchies (it doesn't operate on
partitioned tables, and doesn't have the smarts to direct updates to
traditional inheritance children), and doesn't fire statement
triggers.

In the #2 patch in the other thread about wCTEs, I changed this to
make a TransitionCaptureState object the *only* way to cause
transition tuple capture, because in that patch it owns the
tuplestores without which you can't capture anything.  So in that
patch trigdesc is no longer relevant at all for the tuple capture.
Someone extending execReplication.c to support partitions and
statement triggers etc will need to think about creating a
TransitionCaptureState but I decided that was out of scope for the
transition table rescue project.  In the new version of the #2 patch
that I'm about to post on the other there there is now a comment in
execReplication.c to explain.

> So, in short, +1 from me.

Thanks for the review.  New version of patch #1 attached.

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


transition-tuples-from-child-tables-v11.patch
Description: Binary data

-- 
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] documentation typo in ALTER TABLE example

[Tatsuo Ishii]

> Hi,
> 
> Attached is a simple patch to fix a documentation typo in
> the ALTER TABLE example.

Or the original author's intention might have been something like
this:

--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1399,7 +1399,7 @@ ALTER TABLE cities
Detach a partition from partitioned table:
 
 ALTER TABLE cities
-DETACH PARTITION measurement_y2015m12;
+DETACH PARTITION cities_ab;

Amit Langote, which one was your intention?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

[Masahiko Sawada]

On Fri, Jun 9, 2017 at 10:50 PM, Peter Eisentraut
 wrote:
> On 5/30/17 13:25, Masahiko Sawada wrote:
>> However there is one more problem here; if the relation status entry
>> is deleted while corresponding table sync worker is waiting to be
>> changed its status, the table sync worker can be orphaned in waiting
>> status. In this case, should table sync worker check the relation
>> status and exits if the relation status record gets removed? Or should
>> ALTER SUBSCRIPTION update status of table sync worker to UNKNOWN?
>
> I think this would be fixed with the attached patch.
>

Thank you for the patch. The patch fixes this issue but it takes a
long time to done ALTER SUBSCRIPTION SET PUBLICATION when
max_sync_workers_per_subscription is set high value. Because the
removing entry from pg_subscription_rel and launching a new table sync
worker select a subscription relation state in the same order, the
former doesn't catch up with latter.
For example in my environment, when I test the following step with
max_sync_workers_per_subscription = 15, all table sync workers were
launched once and then killed. How about removing the entry from
pg_subscription_rel in the inverse order?

X cluster ->
=# select 'create table t' || generate_series(1,100) || '(c
int);';\gexec -- create 100 tables
=# create table t (c int);  -- create one more table
=# create publication all_pub for all tables;
=# create publication one_pub for table t;

Y Cluster ->
(create the same 101 tables as well)
=# create subscription hoge_sub connection 'host=localhost  port=5432'
publication all_pub;
=# alter subscription hoge_sub set publication one_pub;  -- execute
this during synchronizing the table, it takes a long time

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


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

[HACKERS] documentation typo in ALTER TABLE example

[Yugo Nagata]

Hi,

Attached is a simple patch to fix a documentation typo in
the ALTER TABLE example.

Regards,

-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 56ea830..4c61c44 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1398,7 +1398,7 @@ ALTER TABLE cities
   
Detach a partition from partitioned table:
 
-ALTER TABLE cities
+ALTER TABLE measurement
 DETACH PARTITION measurement_y2015m12;
 
 

-- 
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] Proposal : For Auto-Prewarm.

[Mithun Cy]

Thanks, Amit,

On Fri, Jun 9, 2017 at 8:07 PM, Amit Kapila  wrote:

>
> Few comments on the latest patch:
> +
> + /* Prewarm buffer. */
> + buf = ReadBufferExtended(rel, blk->forknum, blk->blocknum, RBM_NORMAL,
> + NULL);
> + if (BufferIsValid(buf))
> + ReleaseBuffer(buf);
> +
> + old_blk = blk;
> + ++pos;
>
> You are incrementing position at different places in this loop. I
> think you can do it once at the start of the loop.


Fixed now moved all of ++pos to one place now.


> 2.
> +dump_now(bool is_bgworker)
> {
> ..
> + fd = OpenTransientFile(transient_dump_file_path,
> +   O_CREAT | O_WRONLY | O_TRUNC, 0666);
>
> +prewarm_buffer_pool(void)
> {
> ..
> + file = AllocateFile(AUTOPREWARM_FILE, PG_BINARY_R);
>
> During prewarm, you seem to be using binary mode to open a file
> whereas during dump binary flag is not passed.  Is there a reason
> for such a difference?
>

-- Sorry fixed now, both use binary.


>
> 3.
> + ereport(LOG,
> + (errmsg("saved metadata info of %d blocks", num_blocks)));
>
> It doesn't seem like a good idea to log this info at each dump
> interval.  How about making this as a DEBUG1 message?


-- Fixed, made it as DEBUG1 along with another message "autoprewarm load
task ended" message.


-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


autoprewarm_13.patch
Description: Binary data

-- 
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] Transactional sequence stuff breaks pg_upgrade

[Tom Lane]

I wrote:
> I believe I've identified the reason why skink and some other buildfarm
> members have been failing the pg_upgrade test recently.
> ...
> Not sure what we want to do about it.  One idea is to make
> ALTER SEQUENCE not so transactional when in binary-upgrade mode.

On closer inspection, the only thing that pg_upgrade needs is to be
able to do ALTER SEQUENCE OWNED BY without a relfilenode bump.  PFA
two versions of a patch that fixes this problem, at least to the
extent that it gets through check-world without triggering the Assert
I added to GetNewRelFileNode (which HEAD doesn't).  The first one
is a minimally-invasive hack; the second one puts the responsibility
for deciding if a sequence rewrite is needed into init_params.  That's
bulkier but might be useful if we ever grow additional ALTER SEQUENCE
options that don't need a rewrite.  OTOH, I'm not very clear on what
such options might look like, so maybe the extra flexibility is useless.
Thoughts?

regards, tom lane

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 
--- 391,403 
  	bool		collides;
  	BackendId	backend;
  
+ 	/*
+ 	 * If we ever get here during pg_upgrade, there's something wrong; all
+ 	 * relfilenode assignments during a binary-upgrade run should be
+ 	 * determined by commands in the dump script.
+ 	 */
+ 	Assert(!IsBinaryUpgrade);
+ 
  	switch (relpersistence)
  	{
  		case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 7e85b69..188429c 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*** AlterSequence(ParseState *pstate, AlterS
*** 473,490 
  		GetTopTransactionId();
  
  	/*
! 	 * Create a new storage file for the sequence, making the state changes
! 	 * transactional.  We want to keep the sequence's relfrozenxid at 0, since
! 	 * it won't contain any unfrozen XIDs.  Same with relminmxid, since a
! 	 * sequence will never contain multixacts.
  	 */
! 	RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
! 			  InvalidTransactionId, InvalidMultiXactId);
  
! 	/*
! 	 * Insert the modified tuple into the new storage file.
! 	 */
! 	fill_seq_with_data(seqrel, newdatatuple);
  
  	/* process OWNED BY if given */
  	if (owned_by)
--- 473,499 
  		GetTopTransactionId();
  
  	/*
! 	 * If we are *only* doing OWNED BY, there is no need to rewrite the
! 	 * sequence file nor the pg_sequence tuple; and we mustn't do so because
! 	 * it breaks pg_upgrade by causing unwanted changes in the sequence's
! 	 * relfilenode.
  	 */
! 	if (!(owned_by && list_length(stmt->options) == 1))
! 	{
! 		/*
! 		 * Create a new storage file for the sequence, making the state
! 		 * changes transactional.  We want to keep the sequence's relfrozenxid
! 		 * at 0, since it won't contain any unfrozen XIDs.  Same with
! 		 * relminmxid, since a sequence will never contain multixacts.
! 		 */
! 		RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
!   InvalidTransactionId, InvalidMultiXactId);
  
! 		/*
! 		 * Insert the modified tuple into the new storage file.
! 		 */
! 		fill_seq_with_data(seqrel, newdatatuple);
! 	}
  
  	/* process OWNED BY if given */
  	if (owned_by)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 
--- 391,403 
  	bool		collides;
  	BackendId	backend;
  
+ 	/*
+ 	 * If we ever get here during pg_upgrade, there's something wrong; all
+ 	 * relfilenode assignments during a binary-upgrade run should be
+ 	 * determined by commands in the dump script.
+ 	 */
+ 	Assert(!IsBinaryUpgrade);
+ 
  	switch (relpersistence)
  	{
  		case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 7e85b69..34b8aa2 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*** static Form_pg_sequence_data read_seq_tu
*** 101,107 
  static void init_params(ParseState *pstate, List *options, bool for_identity,
  			bool isInit,
  			Form_pg_sequence seqform,
! 			Form_pg_sequence_data seqdataform, List **owned_by);
  static void do_setval(Oid relid, int64 next, bool iscalled);
  static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);
  
--- 101,109 
  static void init_params(ParseState *pstate, List *options, bool for_identity,
  			bool isInit,
  			Form_pg_sequence seqform,
! 			Form_pg_sequence_data seqdataform,
! 			bool *need_newrelfilenode,
! 			List **owned_by);
  static void do_setval(Oid relid, int64 next, bool iscalled);
  static void 

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Peter Geoghegan]

On Sun, Jun 11, 2017 at 4:10 PM, Tomas Vondra
 wrote:
> I do strongly recommend reading this paper analyzing choke points of
> individual TPC-H queries:
>
> http://oai.cwi.nl/oai/asset/21424/21424B.pdf
>
> It's slightly orthogonal to the issue at hand (poor estimate in Q20 causing
> choice of inefficient plan), it's a great paper to read. I thought I've
> already posted a link to the this paper sometime in the past, but I don't
> see it in the archives.

Thanks for the tip!

The practical focus of this paper really appeals to me.

-- 
Peter Geoghegan


-- 
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] TPC-H Q20 from 1 hour to 19 hours!

[Tomas Vondra]

Hi,

On 6/11/17 7:54 PM, Peter Geoghegan wrote:

On Sun, Jun 11, 2017 at 10:36 AM, Tom Lane  wrote:

Do you mean teaching the optimizer to do something like this?:


Uh, no.  I don't think we want to add any run-time checks.  The point in
this example is that we'd get a better rowcount estimate if we noticed
that the FK constraint could be considered while estimating the size of
the partsupp-to-aggregated-subquery join.


Sorry for not considering the context of the thread more carefully.
Robert said something about selectivity estimation and TPC-H to me,
which I decide to research; I then rediscovered this thread.

Clearly Q20 is designed to reward systems that do better with moving
predicates into subqueries, as opposed to systems with better
selectivity estimation.



I do strongly recommend reading this paper analyzing choke points of 
individual TPC-H queries:


http://oai.cwi.nl/oai/asset/21424/21424B.pdf

It's slightly orthogonal to the issue at hand (poor estimate in Q20 
causing choice of inefficient plan), it's a great paper to read. I 
thought I've already posted a link to the this paper sometime in the 
past, but I don't see it in the archives.


regards

--
Tomas Vondra  http://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] TPC-H Q20 from 1 hour to 19 hours!

[Peter Geoghegan]

On Sun, Jun 11, 2017 at 10:27 AM, Peter Geoghegan  wrote:
> Note that I introduced a new, redundant exists() in the agg_lineitem
> fact table subquery. It now takes 23 seconds for me on Tomas' 10GB
> TPC-H dataset, whereas the original query took over 90 minutes.
> Clearly we're missing a trick or two here. I think that you need a
> DAG-shaped query plan to make this work well, though, so it is
> certainly a big project.

On closer examination, this seems to be due to the number of heap
accesses required by an index-only scan that the original query plan
uses; my test case was invalid. All the same, I understand that moving
predicates into many (TPC-H) subqueries is an important optimization,
and hope that more work can be done in this area.

-- 
Peter Geoghegan


-- 
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] transition table behavior with inheritance appears broken

[Andrew Gierth]

> "Andrew" == Andrew Gierth  writes:

 Andrew> I have it; I will post a status update before 23:59 BST on 11
 Andrew> Jun.

This is that status update.  I am still studying Thomas' latest patch
set; as I mentioned in another message, I've confirmed a memory leak,
and I expect further work may be needed in some other areas as well, but
I think we're still making progress towards fixing it and I will work
with Thomas on it.

I will post a further status update before 23:59 BST on 14th Jun.

-- 
Andrew (irc:RhodiumToad)


-- 
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] SCRAM in the PG 10 release notes

[Álvaro Hernández Tortosa]

On 11/05/17 09:20, Heikki Linnakangas wrote:

On 05/11/2017 07:03 AM, Michael Paquier wrote:
On Thu, May 11, 2017 at 11:50 AM, Bruce Momjian  
wrote:

I have added this as an open item because we will have to wait to see
where we are with driver support as the release gets closer.


As Postgres ODBC now has a hard dependency with libpq, no actions is
taken from there. At least this makes one driver worth mentioning.


FWIW, I wrote a patch for the Go driver at https://github.com/lib/pq, 
to implement SCRAM. It's awaiting review.


I updated the List of Drivers in the Wiki. I added a few drivers that 
were missing, like the ODBC driver, and the pgtclng driver, as well as 
a Go and Rust driver that I'm aware of. I reformatted it, and added a 
column to indicate whether each driver uses libpq or not.


https://wiki.postgresql.org/wiki/List_of_drivers

There is a similar list in our docs:

https://www.postgresql.org/docs/devel/static/external-interfaces.html

Should we update the list in the docs, adding every driver we know of? 
Or curate the list somehow, adding only more popular drivers? Or 
perhaps add a link to the Wiki page from the docs?


We can use this list in the Wiki to track which drivers implement SCRAM.




I have just submitted a PR to add SCRAM support for pgjdbc: 
https://github.com/pgjdbc/pgjdbc/pull/842


Thread on the pgjdbc-list: 
https://www.postgresql.org/message-id/95dea232-aeeb-d619-e917-abf32b44ef8a%408kdata.com


Hopefully it will be merged soon, and the list of drivers could 
then be updated with this.



Álvaro

--

Álvaro Hernández Tortosa


---
<8K>data



--
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Andrew Gierth]

> "Robert" == Robert Haas  writes:

 Robert> I don't see a reason why MakeTransitionCaptureState needs to
 Robert> force the tuplestores into TopTransactionContext or make them
 Robert> owned by TopTransactionResourceOwner.

Nor do I, and I'm pretty sure it's leaking memory wholesale within a
transaction if you have aborted subxacts.

e.g. a few iterations of this, on a table with an appropriate trigger:

savepoint s1;
insert into foo
  select i, case when i < 10 then repeat('a',100)
else repeat('a',1/(i-10)) end
from generate_series(1,10) i;
rollback to s1;

This is a bit contrived of course but it shows that there's missing
cleanup somewhere, either in the form of poor choice of context or
missing code in the subxact cleanup.

 Robert> I mean, it was like that before, but afterTriggers is a global
 Robert> variable and, potentially, there could still be a pointer
 Robert> accessible through it to a tuplestore linked from it even after
 Robert> the corresponding subtransaction aborted, possibly causing some
 Robert> cleanup code to trip and fall over.  But that can't be used to
 Robert> justify this case, because the TransitionCaptureState is only
 Robert> reached through the PlanState tree; if that goes away, how is
 Robert> anybody going to accidentally follow a pointer to the
 Robert> now-absent tuplestore?

For per-row triggers with transition tables, a pointer to the transition
capture state ends up in the shared-data record in the event queue?

-- 
Andrew.


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

[HACKERS] Transactional sequence stuff breaks pg_upgrade

[Tom Lane]

I believe I've identified the reason why skink and some other buildfarm
members have been failing the pg_upgrade test recently.  It is that
recent changes in sequence support have caused binary-upgrade restore
runs to do some sequence OID/relfilenode assignments without any heed
to the OIDs that pg_upgrade tried to impose on those sequences.  Once
those sequences have relfilenodes other than the intended ones, they
are land mines for all subsequent pg_upgrade-controlled table OID
assignments.

I am not very sure why it's so hard to duplicate the misbehavior; perhaps,
in order to make the failure happen with the current regression tests,
it's necessary for a background auto-analyze to happen and consume some
OIDs (for pg_statistic TOAST entries) at just the wrong time.  However,
I can definitely demonstrate that there are uncontrolled relfilenode
assignments happening during pg_upgrade's restore run.  I stuck an
elog() call into GetNewObjectId(), along with generation of a stack
trace using backtrace(), and here is one example:

[593daad3.4863:2243] LOG:  generated OID 16735
[593daad3.4863:2244] STATEMENT:  
-- For binary upgrade, must preserve pg_class oids
SELECT 
pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('46851'::pg_catalog.oid);


-- For binary upgrade, must preserve pg_type oid
SELECT 
pg_catalog.binary_upgrade_set_next_pg_type_oid('46852'::pg_catalog.oid);

ALTER TABLE "itest10" ALTER COLUMN "a" ADD GENERATED BY DEFAULT AS 
IDENTITY (
SEQUENCE NAME "itest10_a_seq"
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1
);

postgres: postgres regression [local] ALTER TABLE(GetNewObjectId+0xda) 
[0x50397a]
postgres: postgres regression [local] ALTER TABLE(GetNewRelFileNode+0xec) 
[0x52430c]
postgres: postgres regression [local] ALTER 
TABLE(RelationSetNewRelfilenode+0x79) [0x851d59]
postgres: postgres regression [local] ALTER TABLE(AlterSequence+0x1cd) 
[0x5d976d]
postgres: postgres regression [local] ALTER TABLE() [0x75d279]
postgres: postgres regression [local] ALTER TABLE(standard_ProcessUtility+0xb7) 
[0x75dec7]
postgres: postgres regression [local] ALTER TABLE() [0x75cb1d]
postgres: postgres regression [local] ALTER TABLE(standard_ProcessUtility+0xb7) 
[0x75dec7]
postgres: postgres regression [local] ALTER TABLE() [0x759f0b]
postgres: postgres regression [local] ALTER TABLE() [0x75ae91]
postgres: postgres regression [local] ALTER TABLE(PortalRun+0x250) [0x75b740]
postgres: postgres regression [local] ALTER TABLE() [0x757be7]
postgres: postgres regression [local] ALTER TABLE(PostgresMain+0xe08) [0x759968]
postgres: postgres regression [local] ALTER TABLE(PostmasterMain+0x1a99) 
[0x6e21a9]
postgres: postgres regression [local] ALTER TABLE(main+0x6b8) [0x65b958]
/lib64/libc.so.6(__libc_start_main+0xfd) [0x3f3bc1ed1d]
postgres: postgres regression [local] ALTER TABLE() [0x473899]


Judging by when we started to see buildfarm failures, I think that
commit 3d79013b9 probably broke it, but the problem seems latent
in the whole concept of transactional sequence information.

Not sure what we want to do about it.  One idea is to make
ALTER SEQUENCE not so transactional when in binary-upgrade mode.

(I'm also tempted to make GetNewRelFileNode complain if IsBinaryUpgrade
is true, but that's a separate matter.)

In any case, this is a "must fix" problem IMO, so I'll go add it to the
open items list.

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

>>> * the comments get formatted differently for -ts4 than -ts8

Still haven't put any thought into it, so I still don't know what to do
here.

>>> * extra spacing getting inserted for fairly long labels

I think the fix is as easy as not producing the space. I committed that.

>>> * some enum declarations get the phony extra indentation
>>> * surplus indentation for SH_ELEMENT_TYPE * data;

I think this is now fixed.

>>> * comments on the same line are now run together
Indent has worked like for a long time; current pg_bsd_indent does that
as well. It was now uncovered by my removing this line from pgindent:

# Add tab before comments with no whitespace before them (on a tab stop)
$source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm;

> There's also the portability issues: __FBSDID() and bcopy() and
>  [and err.h].

I think that's fixed as well.


I've never been too excited to improve indent and it's increasingly
challenging for me to force myself to work on it now, after I've
invested so much of my spare time into it. So please bear with me if
there are any errors.

-- 
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] Buildfarm failures on woodlouse (in ecpg-check)

[Andrew Dunstan]

On 06/11/2017 11:33 AM, Christian Ullrich wrote:
> Hello,
>
> my buildfarm animal woodlouse (Visual Studio 2013 on Windows 7)
> stopped working correctly some months ago. After Tom kindly prodded me
> into fixing it, I noticed that I had configured it to skip the
> ecpg-check step because one of the tests in the "thread" section (not
> always the same) nearly always failed.
>
> I had set it up in circa 2015, so I reenabled the step to see whether
> anything had changed since, and it started failing again.
>
> Through some trial and error, and with the help of Microsoft's
> Application Verifier tool, I found what I think is the cause: It looks
> like the VS 2013 CRT's setlocale() function is not entirely
> thread-safe; the heap debugging options make it crash when
> manipulating per-thread locale state, and according to the comments
> surrounding that spot in the CRT source, the developers were aware the
> code is fragile.
>
> I crudely hacked a critical section around the setlocale() call in
> pgwin32_setlocale(). After this change, the test does not crash, while
> without it, it crashes every time.
>
> If there is interest in fixing this issue that is apparently limited
> to VS 2013, I will try and produce a proper patch. I notice, however,
> that there is a pthread compatibility layer available that I have no
> experience with at all, and I assume using it is preferred over
> straight Win32?
>
> My hack is attached; please feel free to tell me I'm on the wrong track.
> To build correctly, it requires defining _WIN32_WINNT to be 0x600 or
> above (and using an SDK that knows about InitOnceExecuteOnce()).
>
>

It's certainly worth doing.

I turned off testing ecpg ages ago on bowerbird because I was getting
errors. That's an older version of the toolset.

We already define _WIN32_WINNT to be 0x0600 on all appropriate platforms
(Vista/2008 and above), so I think you could probably just check for
that value.

I have no opinion on the pthread question.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Make ANALYZE more selective about what is a "most common value"?

[Dean Rasheed]

On 11 June 2017 at 20:19, Tom Lane  wrote:
>> The standard way of doing this is to calculate the "standard error" of
>> the sample proportion - see, for example [3], [4]:
>>   SE = sqrt(p*(1-p)/n)
>> Note, however, that this formula assumes that the sample size n is
>> small compared to the population size N, which is not necessarily the
>> case. This can be taken into account by applying the "finite
>> population correction" (see, for example [5]), which involves
>> multiplying by an additional factor:
>>   SE = sqrt(p*(1-p)/n) * sqrt((N-n)/(N-1))
>
> It's been a long time since college statistics, but that wikipedia article
> reminds me that the binomial distribution isn't really the right thing for
> our problem anyway.  We're doing sampling without replacement, so that the
> correct model is the hypergeometric distribution.

Yes that's right.

>  The article points out
> that the binomial distribution is a good approximation as long as n << N.
> Can this FPC factor be justified as converting binomial estimates into
> hypergeometric ones, or is it ad hoc?

No, it's not just ad hoc. It comes from the variance of the
hypergeometric distribution [1] divided by the variance of a binomial
distribution [2] with p=K/N, in the notation of those articles.

This is actually a very widely used formula, used in fields like
analysis of survey data, which is inherently sampling without
replacement (assuming the questioners don't survey the same people
more than once!).

Regards,
Dean


[1] https://en.wikipedia.org/wiki/Hypergeometric_distribution
[2] https://en.wikipedia.org/wiki/Binomial_distribution


-- 
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] Make ANALYZE more selective about what is a "most common value"?

[Tom Lane]

Dean Rasheed  writes:
> I think we should attempt to come up with a more principled approach
> to this, taking into account the table and sample sizes. Here's what I
> found, after a bit of research:

Thanks for doing some legwork on this!

> A common initial rule of thumb is that the value should occur at least
> 10 times in the sample - see, for example [1], [2]. ...
> Note that this says nothing about the margin of error of p, just that
> it is reasonable to treat it as having a normal distribution, which
> then allows the margin of error to be analysed using standard
> techniques.

Got it.  Still, insisting on >= 10 occurrences feels a lot better to me
than insisting on >= 2.  It's interesting that that can be correlated
to whether the margin of error is easily analyzed.

> The standard way of doing this is to calculate the "standard error" of
> the sample proportion - see, for example [3], [4]:
>   SE = sqrt(p*(1-p)/n)
> Note, however, that this formula assumes that the sample size n is
> small compared to the population size N, which is not necessarily the
> case. This can be taken into account by applying the "finite
> population correction" (see, for example [5]), which involves
> multiplying by an additional factor:
>   SE = sqrt(p*(1-p)/n) * sqrt((N-n)/(N-1))

It's been a long time since college statistics, but that wikipedia article
reminds me that the binomial distribution isn't really the right thing for
our problem anyway.  We're doing sampling without replacement, so that the
correct model is the hypergeometric distribution.  The article points out
that the binomial distribution is a good approximation as long as n << N.
Can this FPC factor be justified as converting binomial estimates into
hypergeometric ones, or is it ad hoc?

> This gives rise to the possibility of writing another rule for candidate MCVs:
> If there are Nd distinct values in the table, so that the average
> frequency of occurrence of any particular value is 1/Nd, then the test
>   pmin > 1/Nd

Interesting indeed.  We'd have to be working with our estimated Nd, of
course, not the true Nd.  We know empirically that the estimate usually
lowballs the true value unless our sample is a fairly large fraction of
the whole table.  That would tend to make our cutoff pmin too high,
so that we'd be excluding some candidate MCVs even though a better sample
would show they almost certainly had a frequency more common than average.
That behavior seems fine to me; it'd tend to drop questionable MCVs in
small samples, which is exactly what I think we should be doing.  But it
is probably worth doing some empirical experiments with this rule to see
what we get.

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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Dean Rasheed]

On 11 June 2017 at 16:59, Joe Conway  wrote:
> On 06/11/2017 08:55 AM, Joe Conway wrote:
>> Yeah, I noticed the same while working on the RLS related patch. I did
>> not see anything else in rewriteHandler.c but it is probably worth
>> looking wider for other omissions.
>
> So in particular:
>
> #define RelationIsUsedAsCatalogTable(relation)  \
> ((relation)->rd_options && \
>  ((relation)->rd_rel->relkind == RELKIND_RELATION || \
>   (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \
>  ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
> 8<
>
> Does RELKIND_PARTITIONED_TABLE apply there?
>

Hmm, a quick experiment shows that it won't allow a partitioned table
to be used as a user catalog table, although I'm not sure if that's
intentional. If it is, it doesn't appear to be documented.

I found another RLS-related omission -- RemoveRoleFromObjectPolicy()
doesn't work for partitioned tables, so DROP OWNED BY  will fail
if there are any partitioned tables with RLS.

I also found another couple of omissions in PL/pgSQL --
plpgsql_parse_cwordtype() and build_row_from_class(), so for example
%rowtype doesn't work for partitioned tables.

That's it for my quick once-over the code-base, but I'm not confident
that will have caught everything.

Regards,
Dean


-- 
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] Broken hint bits (freeze)

[Vladimir Borodin]

> 8 июня 2017 г., в 17:03, Amit Kapila  написал(а):
> 
> On Thu, Jun 8, 2017 at 6:49 PM, Dmitriy Sarafannikov
>  wrote:
>> 
>>> Why didn't rsync made the copies on master and replica same?
>> 
>> Because rsync was running with —size-only flag.
>> 
> 
> IIUC the situation, the new WAL and updated pg_control file has been
> copied, but not updated data files due to which the WAL has not been
> replayed on replicas?  If so, why the pg_control file is copied, it's
> size shouldn't have changed?

Because on master pg_upgrade moves $prefix/9.5/data/global/pg_control to 
$prefix/9.5/data/global/pg_control.old and creates new 
$prefix/9.6/data/global/pg_control without making hardlink. When running rsync 
from master to replica rsync sees $prefix/9.6/data/global/pg_control on master 
and checks if it is a hardlink. Since it is not a hardlink and 
$prefix/9.6/data/global/pg_control does not exist on replica rsync copies it. 
For data files the logic is different since they are hardlinks, corresponding 
files exist on replica and they are the same size.

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


--
May the force be with you…
https://simply.name

Re: [HACKERS] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

[Tom Lane]

Alvaro Herrera  writes:
> Interesting stuff.  Here's a small recommendation for a couple of those
> new messages.

Hm.  I don't object to folding those two messages into one, but now that
I look at it, the text needs some more work anyway, perhaps.  What we're
actually checking is not so much whether the IS DISTINCT FROM construct
returns a set as whether the underlying equality operator does.  If we
want to be pedantic about it, we'd end up writing something like

"equality operator used by %s must not return a set"

But perhaps it's okay to fuzz the distinction and just write

"%s must not return a set"

You could justify that on the reasoning that if we were to allow this
then an underlying "=" that returned a set would presumably cause
IS DISTINCT FROM or NULLIF to also return a set.

I'm kind of thinking that the second wording is preferable, but there's
room to argue that the first is more precise.  Opinions?

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

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Peter Geoghegan]

On Sun, Jun 11, 2017 at 10:36 AM, Tom Lane  wrote:
>> Do you mean teaching the optimizer to do something like this?:
>
> Uh, no.  I don't think we want to add any run-time checks.  The point in
> this example is that we'd get a better rowcount estimate if we noticed
> that the FK constraint could be considered while estimating the size of
> the partsupp-to-aggregated-subquery join.

Sorry for not considering the context of the thread more carefully.
Robert said something about selectivity estimation and TPC-H to me,
which I decide to research; I then rediscovered this thread.

Clearly Q20 is designed to reward systems that do better with moving
predicates into subqueries, as opposed to systems with better
selectivity estimation.

-- 
Peter Geoghegan


-- 
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] TPC-H Q20 from 1 hour to 19 hours!

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jun 1, 2017 at 8:40 AM, Tom Lane  wrote:
>> The thing that would actually have a chance of improving matters for Q20
>> would be if we could see our way to looking through the aggregation
>> subquery and applying the foreign key constraint for lineitem.  That
>> seems like a research project though; it's surely not happening for v10.

> Do you mean teaching the optimizer to do something like this?:

Uh, no.  I don't think we want to add any run-time checks.  The point in
this example is that we'd get a better rowcount estimate if we noticed
that the FK constraint could be considered while estimating the size of
the partsupp-to-aggregated-subquery join.

> Apparently selectivity estimation isn't particularly challenging with
> the TPC-H queries.

Maybe not with the rest of them, but we're certainly having an issue there
with Q20.

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

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Peter Geoghegan]

On Thu, Jun 1, 2017 at 8:40 AM, Tom Lane  wrote:
> The thing that would actually have a chance of improving matters for Q20
> would be if we could see our way to looking through the aggregation
> subquery and applying the foreign key constraint for lineitem.  That
> seems like a research project though; it's surely not happening for v10.

Do you mean teaching the optimizer to do something like this?:

select
ps_suppkey
from
partsupp,
(
select
l_partkey agg_partkey,
l_suppkey agg_suppkey
from
lineitem
/* BEGIN my addition */
where exists (
select
p_partkey
from
part
where
p_name like 'hot%'
and p_partkey = l_partkey
  )
 /* END my addition */
group by
l_partkey,
l_suppkey
) agg_lineitem
where
agg_partkey = ps_partkey
and agg_suppkey = ps_suppkey
and ps_partkey in (
select
p_partkey
from
part
where
p_name like 'hot%'
);

Note that I introduced a new, redundant exists() in the agg_lineitem
fact table subquery. It now takes 23 seconds for me on Tomas' 10GB
TPC-H dataset, whereas the original query took over 90 minutes.
Clearly we're missing a trick or two here. I think that you need a
DAG-shaped query plan to make this work well, though, so it is
certainly a big project.

Apparently selectivity estimation isn't particularly challenging with
the TPC-H queries. I think that the big challenge for us is
limitations like this; there are similar issues with a number of other
TPC-H queries. It would be great if someone looked into implementing
bitmap semi-join.

-- 
Peter Geoghegan


-- 
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] regproc and when to schema-qualify

[Tom Lane]

Chapman Flack  writes:
> The manual says regproc "will display schema-qualified names on output
> if the object would not be found in the current search path without
> being qualified."

That's less than the full truth :-(

> Is regproc displaying the schema in this case because there are two
> overloaded flavors of ginarrayextract, though both are in pg_catalog?

Yes, see the test in regprocout:

 * Would this proc be found (uniquely!) by regprocin? If not,
 * qualify it.

Of course, in a situation like this, schema-qualification is not enough to
save the day; regprocin will still fail because the name is ambiguous.
You really need to use regprocedure not regproc if you want any guarantees
about the results.

(The fact that we have regproc at all is a bit of a historical accident,
caused by some limitations of the bootstrap mode.)

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

Re: [HACKERS] Make ANALYZE more selective about what is a "most common value"?

[Dean Rasheed]

On 05/06/17 09:30, Tom Lane wrote:
> First, I think we need a larger hard floor on the number of occurrences
> of a value that're required to make ANALYZE decide it is a "most common
> value"...
>
> Second, the code also has a rule that potential MCVs need to have an
> estimated frequency at least 25% larger than what it thinks the "average"
> value's frequency is.  A rule of that general form seems like a good idea,
> but I now think the 25% threshold is far too small to do anything useful.
> In particular, in any case like this where there are more distinct values
> than there are sample rows, the "average frequency" estimate will
> correspond to less than one occurrence in the sample, so that this rule is
> totally useless to filter anything that we would otherwise consider as an
> MCV.  I wonder if we shouldn't make it be "at least double the estimated
> average frequency".
>

I think we should attempt to come up with a more principled approach
to this, taking into account the table and sample sizes. Here's what I
found, after a bit of research:

A common initial rule of thumb is that the value should occur at least
10 times in the sample - see, for example [1], [2]. The idea behind
that goes as follows:

Suppose that N is the population size (total number of rows in the
table), and n is the sample size, and that some particular candidate
value appears x times in the sample.

Then the "sample proportion" is given by

  p = x/n.

Taking a different sample would, in general, give a different value
for x, and hence for the sample proportion, so repeatedly taking
different random samples of the table would lead to a distribution of
values of p, and in general, this distribution would be a binomial
distribution, since x is the count of sampled rows matching a yes/no
question (does the row match the candidate value?).

The idea behind the rule of thumb above is that if n is large enough,
and x is not too close to 0 or n, then the binomial distribution of p
can be reasonably approximated by a normal distribution. There are
various rules of thumb for this to be true, but this one is actually
one of the stronger ones, as can be seen in [2]. Technically the rule
should be:

  x >= 10 and x <= n-10

but it's probably not worth bothering with the second test, since
we're looking for MCVs.

Note that this says nothing about the margin of error of p, just that
it is reasonable to treat it as having a normal distribution, which
then allows the margin of error to be analysed using standard
techniques.

The standard way of doing this is to calculate the "standard error" of
the sample proportion - see, for example [3], [4]:

  SE = sqrt(p*(1-p)/n)

Note, however, that this formula assumes that the sample size n is
small compared to the population size N, which is not necessarily the
case. This can be taken into account by applying the "finite
population correction" (see, for example [5]), which involves
multiplying by an additional factor:

  SE = sqrt(p*(1-p)/n) * sqrt((N-n)/(N-1))

This correction factor becomes 0 when n=N (whole table sampled => no
error) and it approaches 1 when n is much smaller than N.

Having computed the standard error of the sample proportion, it is
then possible to establish a confidence interval around p. For
example, one could say that there is a 95% probability that the total
population proportion lies in the range

  [ pmin = p-2*SE, pmax = p+2*SE ]

This gives rise to the possibility of writing another rule for candidate MCVs:

If there are Nd distinct values in the table, so that the average
frequency of occurrence of any particular value is 1/Nd, then the test

  pmin > 1/Nd

would imply that there is a 97.5% probably that the candidate value is
more common than the average (since the 5% of the distribution of p
outside the confidence interval is split evenly below pmin and above
pmax).

To take a concrete example, suppose that the table has N=1000,000
rows, with Nd=500 distinct values, so that each value occurs on
average 2000 times. If the sample size is 30,000 then the current rule
that a value needs to have an estimated frequency 25% over the average
means that a value would need to be seen 75 times to be considered as
an MCV. If that rule were changed to "at least double the estimated
average frequency", then a value would need to be seen at least 150
times. On the other hand, using the above rule based on a 95%
confidence interval, a value would only need to be seen 78 times, in
which case the estimated total number of occurrences of the value in
the table would be 2600+/-579, and using the MCV estimate would almost
certainly be better than not using it.

Regards,
Dean


[1] https://onlinecourses.science.psu.edu/stat200/node/43
[2] https://en.wikipedia.org/wiki/Binomial_distribution#Normal_approximation
[3] http://mathworld.wolfram.com/SampleProportion.html
[4] https://en.wikipedia.org/wiki/Population_proportion
[5] 

[HACKERS] regproc and when to schema-qualify

[Chapman Flack]

I was idly following along in GSoC 2017: Foreign Key Arrays
when I noticed this:

=# select * from pg_amproc where amprocfamily = 2745;
 amprocfamily | amproclefttype | amprocrighttype | amprocnum |
amproc
--++-+---+
 2745 |   2277 |2277 | 2 |
pg_catalog.ginarrayextract
 2745 |   2277 |2277 | 3 |
ginqueryarrayextract
...

where only ginarrayextract is schema-qualified. It seems to be
regproc's output procedure doing it:

=# select 2743::regproc, 2774::regproc;
  regproc   |   regproc
+--
 pg_catalog.ginarrayextract | ginqueryarrayextract


The manual says regproc "will display schema-qualified names on output
if the object would not be found in the current search path without
being qualified."

Is regproc displaying the schema in this case because there are two
overloaded flavors of ginarrayextract, though both are in pg_catalog?
Could it be searching for the object by name, ignoring the argument
signature, and just detecting that it hit one with a different OID first?

-Chap


-- 
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] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/11/2017 08:55 AM, Joe Conway wrote:
> On 06/11/2017 04:32 AM, Dean Rasheed wrote:
>> It looks like relation_is_updatable() didn't get the message about
>> partitioned tables. Thus, for example, information_schema.views and
>> information_schema.columns report that simple views built on top of
>> partitioned tables are non-updatable, which is wrong. Attached is a
>> patch to fix this.
> 
>> I think this kind of omission is an easy mistake to make when adding a
>> new relkind, so it might be worth having more pairs of eyes looking
>> out for more of the same. I did a quick scan of the rewriter code
>> (prompted by the recent similar omission for RLS on partitioned
>> tables) and I didn't find any more problems there, but I haven't
>> looked elsewhere yet.
> 
> Yeah, I noticed the same while working on the RLS related patch. I did
> not see anything else in rewriteHandler.c but it is probably worth
> looking wider for other omissions.

So in particular:

src/include/utils/rel.h:318:
8<
/*
 * RelationIsUsedAsCatalogTable
 *Returns whether the relation should be treated as a catalog table
 *from the pov of logical decoding.  Note multiple eval of argument!
 */
#define RelationIsUsedAsCatalogTable(relation)  \
((relation)->rd_options && \
 ((relation)->rd_rel->relkind == RELKIND_RELATION || \
  (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \
 ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
8<

Does RELKIND_PARTITIONED_TABLE apply there?

Will continue to poke around...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/11/2017 04:32 AM, Dean Rasheed wrote:
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.

> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.

Yeah, I noticed the same while working on the RLS related patch. I did
not see anything else in rewriteHandler.c but it is probably worth
looking wider for other omissions.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/09/2017 02:52 PM, Joe Conway wrote:
> On 06/09/2017 06:16 AM, Joe Conway wrote:
>> On 06/08/2017 11:09 PM, Noah Misch wrote:
>>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
 On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
 > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
 > in order to demonstrate policy sorting and order of RLS/partition
 > constraint checking. It should be much more straight-forward now, but
 > let me know if there are any further recommended changes.
 
 Thanks, will take a look towards the end of the day.
>>> 
>>> This PostgreSQL 10 open item is past due for your status update.  Kindly 
>>> send
>>> a status update within 24 hours, and include a date for your subsequent 
>>> status
>>> update.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>> 
>> I started reviewing the latest patch last night and will try to finish
>> up this afternoon (west coast USA time).
> 
> I left the actual (2 line) code change untouched, but I tweaked the
> regression test changes a bit. If there are no complaints I will push
> tomorrow (Saturday).

I waited until Sunday, but pushed none-the-less.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Buildfarm failures on woodlouse (in ecpg-check)

[Christian Ullrich]

Hello,

my buildfarm animal woodlouse (Visual Studio 2013 on Windows 7) stopped 
working correctly some months ago. After Tom kindly prodded me into 
fixing it, I noticed that I had configured it to skip the ecpg-check 
step because one of the tests in the "thread" section (not always the 
same) nearly always failed.


I had set it up in circa 2015, so I reenabled the step to see whether 
anything had changed since, and it started failing again.


Through some trial and error, and with the help of Microsoft's 
Application Verifier tool, I found what I think is the cause: It looks 
like the VS 2013 CRT's setlocale() function is not entirely thread-safe; 
the heap debugging options make it crash when manipulating per-thread 
locale state, and according to the comments surrounding that spot in the 
CRT source, the developers were aware the code is fragile.


I crudely hacked a critical section around the setlocale() call in 
pgwin32_setlocale(). After this change, the test does not crash, while 
without it, it crashes every time.


If there is interest in fixing this issue that is apparently limited to 
VS 2013, I will try and produce a proper patch. I notice, however, that 
there is a pthread compatibility layer available that I have no 
experience with at all, and I assume using it is preferred over straight 
Win32?


My hack is attached; please feel free to tell me I'm on the wrong track.
To build correctly, it requires defining _WIN32_WINNT to be 0x600 or 
above (and using an SDK that knows about InitOnceExecuteOnce()).


--
Christian
diff --git a/src/port/win32setlocale.c b/src/port/win32setlocale.c
index cbf109836b..278d836b4d 100644
--- a/src/port/win32setlocale.c
+++ b/src/port/win32setlocale.c
@@ -164,19 +164,33 @@ map_locale(const struct locale_map * map, const char 
*locale)
return locale;
 }
 
+static CRITICAL_SECTION setlocale_cs;
+static INIT_ONCE init_once;
+static BOOL CALLBACK init_setlocale_cs(PINIT_ONCE pInitOnce, PVOID pParam, 
PVOID pCtx)
+{
+   InitializeCriticalSection((PCRITICAL_SECTION)pParam);
+   return TRUE;
+}
+
 char *
 pgwin32_setlocale(int category, const char *locale)
 {
const char *argument;
char   *result;
 
+   if (InitOnceExecuteOnce(_once, init_setlocale_cs, 
(PVOID)_cs, NULL) == 0) {
+   abort();
+   }
+
if (locale == NULL)
argument = NULL;
else
argument = map_locale(locale_map_argument, locale);
 
/* Call the real setlocale() function */
+   EnterCriticalSection(_cs);
result = setlocale(category, argument);
+   LeaveCriticalSection(_cs);
 
/*
 * setlocale() is specified to return a "char *" that the caller is

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

[HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Dean Rasheed]

Hi,

It looks like relation_is_updatable() didn't get the message about
partitioned tables. Thus, for example, information_schema.views and
information_schema.columns report that simple views built on top of
partitioned tables are non-updatable, which is wrong. Attached is a
patch to fix this.

I think this kind of omission is an easy mistake to make when adding a
new relkind, so it might be worth having more pairs of eyes looking
out for more of the same. I did a quick scan of the rewriter code
(prompted by the recent similar omission for RLS on partitioned
tables) and I didn't find any more problems there, but I haven't
looked elsewhere yet.

Regards,
Dean


teach-relation-is-updatable-about-partitioned-tables.patch
Description: Binary data

-- 
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] RTE_NAMEDTUPLESTORE, enrtuples and comments

[Thomas Munro]

On Sun, Jun 11, 2017 at 6:25 PM, Noah Misch  wrote:
> While completing my annual src/backend/nodes/*funcs.c audit, I noticed defects
> in commit 18ce3a4 changes to RangeTblEntry:
>
> 1. Field relid is under a comment saying it is valid for RTE_RELATION only.

The comment is out of date.  Here's a fix for that.

>Fields coltypes, coltypmods and colcollations are under a comment saying
>they are valid for RTE_VALUES and RTE_CTE only.  But _outRangeTblEntry()
>treats all of the above as valid for RTE_NAMEDTUPLESTORE.  Which is right?

The comment is wrong.  In passing I also noticed that RTE_TABLEFUNC
also uses coltypes et al and is not mentioned in that comment.  Here's
a fix for both omissions.

> 2. New fields enrname and enrtuples are set only for RTE_NAMEDTUPLESTORE, yet
>they're under the comment for RTE_VALUES and RTE_CTE.  This pair needs its
>own comment.

Right.  The attached patch fixes that too.

> 3. Each of _{copy,equal,out,read}RangeTblEntry() silently ignores enrtuples.
>_equalRangeTblEntry() ignores enrname, too.  In each case, the function
>should either use the field or have a comment to note that skipping the
>field is intentional.  Which should it be?

Ignoring enrname in _equalRangeTblEntry is certainly a bug, and the
attached adds it.  I also noticed that _copyRangeTleEntry had enrname
but not in the same order as the struct's members, which seems to be
an accidental deviation from the convention, so I moved it in the
attached.

Ignoring enrtuples is probably also wrong, but ...

> This fourth point is not necessarily a defect: I wonder if RangeTblEntry is
> the right place for enrtuples.  It's a concept regularly seen in planner data
> structures but not otherwise seen at parse tree level.

I agree that this is strange.  Perhaps
set_namedtuplestore_size_estimates should instead look up the
EphemeralNamedRelation by rte->enrname to find its way to
enr->md.enrtuples, but I'm not sure off the top of my head how it
should get its hands on the QueryEnvironment required to do that.  I
will look into this on Monday, but other ideas/clues welcome...

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


fixes-for-enr-rte-review-v1.patch
Description: Binary data

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

[HACKERS] RTE_NAMEDTUPLESTORE, enrtuples and comments

[Noah Misch]

While completing my annual src/backend/nodes/*funcs.c audit, I noticed defects
in commit 18ce3a4 changes to RangeTblEntry:

1. Field relid is under a comment saying it is valid for RTE_RELATION only.
   Fields coltypes, coltypmods and colcollations are under a comment saying
   they are valid for RTE_VALUES and RTE_CTE only.  But _outRangeTblEntry()
   treats all of the above as valid for RTE_NAMEDTUPLESTORE.  Which is right?

2. New fields enrname and enrtuples are set only for RTE_NAMEDTUPLESTORE, yet
   they're under the comment for RTE_VALUES and RTE_CTE.  This pair needs its
   own comment.

3. Each of _{copy,equal,out,read}RangeTblEntry() silently ignores enrtuples.
   _equalRangeTblEntry() ignores enrname, too.  In each case, the function
   should either use the field or have a comment to note that skipping the
   field is intentional.  Which should it be?

This fourth point is not necessarily a defect: I wonder if RangeTblEntry is
the right place for enrtuples.  It's a concept regularly seen in planner data
structures but not otherwise seen at parse tree level.

nm


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