Re: [WIP] [B-Tree] Retail IndexTuple deletion

[Kuntal Ghosh]

On Wed, Jun 27, 2018 at 12:10 PM, Andrey V. Lepikhov
 wrote:
> I prepare third version of the patches. Summary:
> 1. Mask DEAD tuples at a page during consistency checking (See comments for
> the mask_dead_tuples() function).

- ItemIdSetDead(lp);
+ if (target_index_deletion_factor > 0)
+ ItemIdMarkDead(lp);
+ else
+ ItemIdSetDead(lp);
IIUC, you want to hold the storage of DEAD tuples to form the ScanKey
which is required for the index scan in the second phase of quick
vacuum strategy. To achieve that, you've only marked the tuple as DEAD
during pruning. Hence, PageRepairFragmentation cannot claim the space
for DEAD tuples since they still have storage associated with them.
But, you've WAL-logged it as DEAD tuples having no storage. So, when
the WAL record is replayed in standby(or crash recovery), the tuples
will be marked as DEAD having no storage and their space may be
reclaimed by PageRepairFragmentation. Now, if you do byte-by-byte
comparison with wal_consistency tool, it may fail even for normal
tuple as well. Please let me know if you feel the same way.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Re: partition tree inspection functions

[Dilip Kumar]

On Tue, Jun 26, 2018 at 10:38 AM, Amit Langote
 wrote:
> Hi.
>
> As discussed a little while back [1] and also recently mentioned [2], here
> is a patch that adds a set of functions to inspect the details of a
> partition tree.  There are three functions:
>
> pg_partition_parent(regclass) returns regclass
> pg_partition_root_parent(regclass) returns regclass
> pg_partition_tree_tables(regclass) returns setof regclass
>
>
> selectp as relname,
>   pg_partition_parent(p) as parent,
>   pg_partition_root_parent(p) as root_parent
> from  pg_partition_tree_tables('p') p;
>  relname | parent | root_parent
> -++-
>  p   || p
>  p0  | p  | p
>  p1  | p  | p
>  p00 | p0 | p
>  p01 | p0 | p
>  p10 | p1 | p
>  p11 | p1 | p
> (7 rows)
>

Is it a good idea to provide a function or an option which can provide
partitions detail in hierarchical order?

i.e
relname level
p 0
p0   1
p00 2
p01 2
p1   1



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [WIP] [B-Tree] Retail IndexTuple deletion

[Andrey V. Lepikhov]

On 29.06.2018 10:00, Kuntal Ghosh wrote:

On Wed, Jun 27, 2018 at 12:10 PM, Andrey V. Lepikhov
 wrote:

I prepare third version of the patches. Summary:
1. Mask DEAD tuples at a page during consistency checking (See comments for
the mask_dead_tuples() function).


- ItemIdSetDead(lp);
+ if (target_index_deletion_factor > 0)
+ ItemIdMarkDead(lp);
+ else
+ ItemIdSetDead(lp);
IIUC, you want to hold the storage of DEAD tuples to form the ScanKey
which is required for the index scan in the second phase of quick
vacuum strategy. To achieve that, you've only marked the tuple as DEAD
during pruning. Hence, PageRepairFragmentation cannot claim the space
for DEAD tuples since they still have storage associated with them.
But, you've WAL-logged it as DEAD tuples having no storage. So, when
the WAL record is replayed in standby(or crash recovery), the tuples
will be marked as DEAD having no storage and their space may be
reclaimed by PageRepairFragmentation. Now, if you do byte-by-byte
comparison with wal_consistency tool, it may fail even for normal
tuple as well. Please let me know if you feel the same way.


Thanks for your analysis.
In this development version of the patch I expect the same prune() 
strategy on a master and standby (i.e. target_index_deletion_factor is 
equal for both).
In this case storage of a DEAD tuple holds during replay or recovery in 
the same way.
On some future step of development I plan to use more flexible prune() 
strategy. This will require to append a 'isDeadStorageHold' field to the 
WAL record.


--
Regards,
Andrey Lepikhov
Postgres Professional:
https://postgrespro.com
The Russian Postgres Company

Re: partitioning - changing a slot's descriptor is expensive

[Amit Khandekar]

On 27 June 2018 at 18:33, Ashutosh Bapat
 wrote:
> On Wed, Jun 27, 2018 at 10:39 AM, Andres Freund  wrote:
>> Hi,
>>
>> (sorry if I CCed the wrong folks, the code has changed a fair bit)
>>
>> I noticed that several places in the partitioning code look like:
>>
>> /*
>>  * If the tuple has been routed, it's been converted to the
>>  * partition's rowtype, which might differ from the root
>>  * table's.  We must convert it back to the root table's
>>  * rowtype so that val_desc shown error message matches the
>>  * input tuple.
>>  */
>> if (resultRelInfo->ri_PartitionRoot)
>> {
>> TableTuple tuple = ExecFetchSlotTuple(slot);
>> TupleConversionMap *map;
>>
>> rel = resultRelInfo->ri_PartitionRoot;
>> tupdesc = RelationGetDescr(rel);
>> /* a reverse map */
>> map = convert_tuples_by_name(orig_tupdesc, tupdesc,
>>  gettext_noop("could not convert row 
>> type"));
>> if (map != NULL)
>> {
>> tuple = do_convert_tuple(tuple, map);
>> ExecSetSlotDescriptor(slot, tupdesc);
>> ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>> }
>> }
>>
>>
>> Unfortunately calling ExecSetSlotDescriptor() is far from cheap, it has
>> to reallocate the values/isnull arrays (and potentially do desc pinning
>> etc).
>
> I bumped into this code yesterday while looking at ExecFetchSlotTuple
> and ExecMaterializeSlot usages. I was wondering the same.
>
> ExecSetSlotDescriptor() always frees tts_values and tts_isnull array
> even if the tuple descriptor being set has same number of attributes
> as previous one. Most of the times that will be the case. I think we
> should optimize that case.

+1

>> I think it'd be good to rewrite the code so there's an input and an
>> output slot that each will keep their slot descriptors set.
>
> +1 for that.
>
> But I am worried that the code will have to create thousand slots if
> there are thousand partitions. I think we will need to see how much
> effect that has.

I agree that it does not make sense to create as many slots, if at all
we go by this approach. Suppose the partitioned table is the only one
having different tuple descriptor, and rest of the partitions have
same tuple descriptor. In that case, keep track of unique descriptors,
and allocate a slot per unique descriptor.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Haribabu Kommi]

On Sat, Jun 23, 2018 at 2:52 PM Ashutosh Sharma 
wrote:

> On Sat, Jun 23, 2018 at 7:36 AM, Haribabu Kommi
>  wrote:
> > On Sat, Jun 23, 2018 at 5:45 AM Euler Taveira 
> wrote:
> >>
> >> 2018-06-22 12:06 GMT-03:00 Robert Haas :
> >> > On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira  >
> >> > wrote:
> >> >> Why don't you extend the existing function
> pg_stat_statements_reset()?
> >> >
> >> > Well, the existing function doesn't take any arguments.  We could add
> >> > an additional version of it that takes an argument, or we could
> >> > replace the existing version with one that has an optional argument.
> >> > But are either of those things any better than just adding a new
> >> > function with a different name, like
> >> > pg_stat_statements_reset_statement()?
> >> >
> >> From the user's POV, overloading is a good goal. It is better to
> >> remember one function name than 3 different function names to do the
> >> same task (reset statement statistics).
> >
> > I agree that function overloading is beneficial until unless it doesn't
> > introduce
> > confusion and difficulty in using it.
> >
> >> > I have not had such good experiences with function overloading, either
> >> > in PostgreSQL or elsewhere, that I'm ready to say reusing the same
> >> > name is definitely the right approach.  For example, suppose we
> >> > eventually end up with a function that resets all the statements, a
> >> > function that resets just one statement, a function that resets all
> >> > statements for one user, and a function that resets all statements
> >> > where the statement text matches a certain regexp.  If those all have
> >> > separate names, everything is fine.  If they all have the same name,
> >> > there's no way that's not confusing.
> >> >
>
> I'm slightly confused about the function overloading part here (not
> sure whether we should try using the same function name or introduce a
> function with new name), however, I think, passing userid, dbid and
> queryid as input arguments to the user function (may be
> pg_stat_statements_reset_statement or pg_stat_statements_reset) looks
> like a good option as it would allow users to remove an entry for a
> particular query across the databases not just in the database that
> the user is currently connected to. The current patch definitely lacks
> this flexibility.
>

Thanks for all your comments on the and approach. Patch is changed
to use the existing _reset function and it takes 3 arguments userid, dbid
and queryid
and their default values are 0.

If all values are passed, it resets only a single query statement,
otherwise it resets
all the statements that matches with the other parameters. If no values are
passed
or all values are invalid, all the statistics are reset.

Updated patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-pg_stat_statements_reset-to-reset-specific-query-use.patch
Description: Binary data

Re: Does logical replication supports cross platform servers?

[Haribabu Kommi]

On Thu, Jun 28, 2018 at 11:12 PM Bruce Momjian  wrote:

> On Wed, Jun 27, 2018 at 11:16:07PM +1000, Haribabu Kommi wrote:
> > On Sun, Jun 24, 2018 at 6:36 AM Bruce Momjian  wrote:
> >
> > On Wed, Jun 13, 2018 at 11:42:14AM +1000, Haribabu Kommi wrote:
> > >
> > > Thanks for the confirmation.
> > > This is a good use case that must be explicitly mentioned in the
> docs.
> > > How about the attached patch?
> >
> > OK, doc patch added to git head.  Let me know if I should back patch
> it
> > further.  Thanks.
> >
> >
> > Thanks for the commit.
> > Yes, it needs to be back patched till 10 stable.
>
> Sure, done, thanks.
>

Thanks.

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Amit Langote]

On 2018/06/29 6:23, Peter Eisentraut wrote:
> On 6/28/18 22:52, Alvaro Herrera wrote:
>>> Couldn't psql chase the pg_depend links to find inherited triggers?
>>
>> Yeah, I thought this would be exceedingly ugly, but apparently it's not
>> *that* bad -- see the attached patch, which relies on the fact that
>> triggers don't otherwise depend on other triggers.  We don't use this
>> technique elsewhere in psql though.
> 
> Yeah, relying on pg_depend to detect relationships between catalog
> objects is a bit evil.  We do use this for detecting sequences linked to
> tables, which also has its share of problems.  Ideally, there would be a
> column in pg_trigger, perhaps, that makes this link explicit.  But we
> are looking here for a solution without catalog changes, I believe.

+1 if that gets the job done.

Thanks,
Amit

Re: Make deparsing of column defaults faster

[Jeff Janes]

On Mon, Jun 4, 2018 at 10:00 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/4/18 20:55, Jeff Janes wrote:
> > The user docs say about column defaults: "The value is any variable-free
> > expression (subqueries and cross-references to other columns in the
> > current table are not allowed)"
> >
> > And also say about pg_get_expr "If the expression might contain Vars,
> > specify the OID of the relation they refer to as the second parameter;
> > if no Vars are expected, zero is sufficient"
> >
> > Since defaults can't contain Vars, this patch converts the second
> > parameter to zero in places where pg_get_expr is invoked on column
> > defaults.
>
> My in-progress generated columns patch removes that assumption (since a
> generated column generally refers to another column of the same table).
>

Will your patch put the generated columns data into pg_attrdef, or create a
new catalog to hold them?  I guess it would make sense to re-use the
catalog since you can't have both a default value and be a generated column.


> > Doing this drops the time needed to run `pg_dump -s` on a
> > 1600 column table with defaults on all columns by a factor of 40.  So
> > pg_upgrade users with such tables should see a big win (cc Justin).
>
> That's impressive but also a bit disturbing.  Where does this
> performance change come from?  Can we harness it in different ways?
>

The most fundamental way would be to change make_colname_unique to use a
hash table, rather than an N^2 algorithm.  It would probably be similar to
Tom's commit 8004953b5a2449c26c4e "Speed up ruleutils' name de-duplication
code" but he does warn in the commit message that his method would be
trickier to use for column aliases than table aliases.  That would speed up
a lot more cases than just DEFAULTS (check constraints, maybe triggers) ,
but would be harder to do.

Even once we fix this, it still wouldn't make sense to create a deparse
context which cannot be necessary.  Perhaps the call to deparse_context_for
could be made lazy, so that it is delayed until a Var is actually found in
the expression node tree, rather than being done preemptively.  Then all
cases without Vars would be improved, not just cases where it could be
proved by static code analysis that Vars are not possible.

When setting up the deparse context for a single table, the column names
already need to be unique (I think).  So another possibility is to pass a
flag down from set_simple_column_names to set_relation_column_names to
indicate no unique checking is needed.

Since pg_dump calls pg_get_expr once over and over again on the same table
consecutively, perhaps we could cache the column alias assignments in a
single-entry cache, so if it is called on the same table as last time it
just re-uses the aliases from last time.  I am not planning on working on
that, I don't know where such a cache could be stored such that is freed
and invalidated at the appropriate times.

Cheers,

Jeff

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Yugo Nagata]

On Thu, 28 Jun 2018 16:22:15 -0700
"David G. Johnston"  wrote:

> On Thu, Jun 28, 2018 at 3:57 PM, Daniel Gustafsson  wrote:

Thank you for your reviewing!

I attached the updated patch.

> 
> > > On 27 Jun 2018, at 18:02, Yugo Nagata  wrote:
> >
> > > I found that there isn't explanation about EXCLUDING in CREATE TABLE doc.
> > > Attached is a patch to add this. I would appreciate it if a native
> > English
> > > speaker comments on this.
> >
> > +  If EXCLUDING option  is
> > specified
> >
> > The empty  seems wrong.

Fixed

> >
> > +  after INCLUDING options, the specified thing is
> > excluded
> >
> > “thing” sounds a bit vague here (and in the last sentence as well), but I’m
> > also not sure what to use instead.  “referenced objects" perhaps?

Fixed.

> >
> > +1 on documenting the EXCLUDING option though.
> >
> 
> ​"is excluded" and "not copied" are redundant to each other and the first

I removed "is excluded".

> sentence is basically redundant with the second.
> 
> ​Maybe try something like:
> 
> It is legal to specify the same option multiple times - e.g., "INCLUDING
> option EXCLUDING option" - the outcome is whichever comes last in the
> command (i.e., in the example, option is excluded).

Certainly. However, it seems to me that example is also redundant.
I rewrote this as follows:

 It is legal to specify multiple options for the same kind of object. 
 If they conflict, latter options always override former options. 

Does this make sense?

> 
> David J.


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 2a1eac9..d57af73 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -624,6 +624,13 @@ WITH ( MODULUS numeric_literal, REM
   INCLUDING COMMENTS INCLUDING CONSTRAINTS INCLUDING DEFAULTS INCLUDING IDENTITY INCLUDING INDEXES INCLUDING STATISTICS INCLUDING STORAGE.
  
  
+  If EXCLUDING option is specified after
+  INCLUDING option, the referenced objects are not copied.
+  This is tipically used after INCLUDING ALL.  
+  It is legal to specify multiple options for the same kind of object.
+  If they conflict, latter options always override former options.
+ 
+ 
   Note that unlike INHERITS, columns and
   constraints copied by LIKE are not merged with similarly
   named columns and constraints.

cost_sort() improvements

[Teodor Sigaev]

Hi!

Current estimation of sort cost has following issues:
 - it doesn't differ one and many columns sort
 - it doesn't pay attention to comparison function cost and column width
 - it doesn't try to count number of calls of comparison function on per column
   basis

At least [1] and [2] hit into to that issues and have an objections/questions 
about correctness of cost sort estimation. Suggested patch tries to improve 
current estimation and solve that issues.


Basic ideas:
 - let me introduce some designations
i   - index of column, started with 1
N   - number of tuples to sort
Gi  - number of groups, calculated by i number columns. Obviously,
  G0 == 1. Number of groups is caculated by estimate_num_groups().
NGi - number of tuples in one group. NG0 == N and NGi = N/G(i-1)
Fi  - cost of comparison function call of i-th column
Wi  - average width in bytes of 1-ith column.
Ci  - total cost of one comparison call of column calculated as
  Fi * fine_function(Wi) where fine_function(Wi) looks like:
  if (Wi <= sizeof(Datum))
  return 1.0; //any type passed in Datum
  else
  return 1 + log16(Wi/sizeof(Datum));
  log16 just an empirical choice
 - So, cost for first column is 2.0 * C0 * log2(N)
   First column is always compared, multiplier 2.0 is defined per regression
   test. Seems, it estimates a cost for transferring tuple to CPU cache,
   starting cost for unpacking tuple, cost of call qsort compare wrapper
   function, etc. Removing this multiplier causes too optimistic prediction of
   cost.
 - cost for i-th columns:
   Ci * log2(NGi) => Ci * log2(N/G(i-1))
   So, here I believe, that i-th comparison function will be called only inside
   group which number is defined by (i-1) leading columns. Note, following
   discussion [1] I add multiplier 1.5 here to be closer to worst case when
   groups are significantly differ. Right now there is no way to determine how
   differ they could be. Note, this formula describes cost of first column too
   except difference with multiplier.
 - Final cost is cpu_operator_cost * N * sum(per column costs described above).
   Note, for single column with width <= sizeof(datum) and F1 = 1 this formula
   gives exactly the same result as current one.
 - for Top-N sort empiric is close to old one: use 2.0 multiplier as constant
   under log2, and use log2(Min(NGi, output_tuples)) for second and following
   columns.

I believe, suggested method could be easy enhanced to support [1] and used in 
[2].

Open items:
 - Fake Var. I faced with generate_append_tlist() which generates Var with
   varno = 0, it was invented, as I can see, at 2002 with comment 'should be
   changed in future'. Future hasn't yet come... I've added workaround to
   prevent call estimate_num_group() with such Vars, but I'm not sure that is
   enough.
 - Costs of all comparison functions now are the same and equal to 1. May be,
   it's time to change that.
 - Empiric constants. I know, it's impossible to remove them at all, but, may
   be, we need to find better estimation of them.

[1] https://commitfest.postgresql.org/18/1124/
[2] https://commitfest.postgresql.org/18/1651/

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index a2a7e0c520..d6b19bb9ad 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1612,6 +1612,231 @@ cost_recursive_union(Path *runion, Path *nrterm, Path *rterm)
 	rterm->pathtarget->width);
 }
 
+/*
+ * is_fake_var
+ *		Workaround for generate_append_tlist() which generates fake Vars with
+ *		varno == 0, that will cause a fail of estimate_num_group() call
+ */
+static bool
+is_fake_var(Expr *expr)
+{
+	if (IsA(expr, RelabelType))
+		expr = (Expr *) ((RelabelType *) expr)->arg;
+
+	return (IsA(expr, Var) && ((Var *) expr)->varno == 0);
+}
+
+/*
+ * get_width_cost_multiplier
+ *		Returns relative complexity of comparing two valyes based on it's width.
+ * The idea behind - long values have more expensive comparison. Return value is
+ * in cpu_operator_cost unit.
+ */
+static double
+get_width_cost_multiplier(PlannerInfo *root, Expr *expr)
+{
+	double	width = -1.0; /* fake value */
+
+	if (IsA(expr, RelabelType))
+		expr = (Expr *) ((RelabelType *) expr)->arg;
+
+	/* Try to find actual stat in corresonding relation */
+	if (IsA(expr, Var))
+	{
+		Var		*var = (Var *) expr;
+
+		if (var->varno > 0 && var->varno < root->simple_rel_array_size)
+		{
+			RelOptInfo	*rel = root->simple_rel_array[var->varno];
+
+			if (rel != NULL &&
+var->varattno >= rel->min_attr &&
+var->varattno <= rel->max_attr)
+			{
+int	ndx = var->varattno - rel->min_attr;
+
+if (rel->attr_widths[ndx] > 0)
+	width = rel->attr_widths[ndx];
+			}
+		}
+	}
+
+	/* 

Re: Server crashed with "TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)"

[Andres Freund]

Hi,

On 2018-06-28 16:08:31 +0530, Rajkumar Raghuwanshi wrote:
> I am getting server crash with below test case, logfile message and core
> dump details in the mail.

Which versions are affected here? Is this reproducible in postgres 10 or
just the current beta?


> postgres=# CREATE TABLE part_tbl (a INT, b TEXT, c INT) PARTITION BY
> RANGE(a);
> CREATE TABLE
> postgres=# CREATE TABLE part_tbl_p PARTITION OF part_tbl FOR VALUES FROM
> (minvalue) TO (maxvalue);
> CREATE TABLE
> postgres=# CREATE INDEX partidx_abc_idx ON part_tbl (a, b, c);
> CREATE INDEX
> postgres=# INSERT INTO part_tbl (a, b, c) SELECT i,i,i FROM
> generate_series(1,50)i;
> INSERT 0 50
> postgres=# ANALYZE part_tbl;
> ANALYZE
> postgres=# ALTER TABLE part_tbl ALTER COLUMN c TYPE numeric;
> 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.
> !>
> 
> 
> --logfile
> TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)
> 2018-06-28 00:01:01.679 IST [97062] LOG:  server process (PID 97080) was
> terminated by signal 6: Aborted
> 2018-06-28 00:01:01.679 IST [97062] DETAIL:  Failed process was running:
> ALTER TABLE part_tbl ALTER COLUMN c TYPE numeric;

Likely a memory lifetime issue or something like that.

Greetings,

Andres Freund

Re: Server crashed with "TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)"

[Andres Freund]

Hi,

On 2018-06-28 22:35:11 +0530, Rajkumar Raghuwanshi wrote:
> On Thu, Jun 28, 2018 at 9:45 PM, Andres Freund  wrote:
> > On 2018-06-28 16:08:31 +0530, Rajkumar Raghuwanshi wrote:
> > > I am getting server crash with below test case, logfile message and core
> > > dump details in the mail.
> >
> > Which versions are affected here? Is this reproducible in postgres 10 or
> > just the current beta?
> >
> 
> This affected current beta only as this is not reproducible in postgres 10
> because index create on
> partitioned table is not supported in v10.

Alvaro, sounds like it could be an open item assigned to you?

Greetings,

Andres Freund

Re: Tips on committing

[Alvaro Herrera]

On 2018-Jun-28, Peter Geoghegan wrote:

> On Thu, Jun 28, 2018 at 8:21 AM, Bruce Momjian  wrote:
> > Good point.  I have never used it but I can see its value.  I have added
> > it to my template.
> 
> FWIW, I developed a document on committing for my own reference, with
> some help from Andres.

Sounds very useful.

How about turning it into a wiki page, for everybody's benefit?

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

Re: POC: GROUP BY optimization

[Teodor Sigaev]

I'm afraid that's a misunderstanding - my objections did not really go
away. I'm just saying my claim that we're not messing with order of
grouping keys is not entirely accurate, because we do that in one
particular case.

I still think we need to be careful when introducing new optimizations
in this area - reordering the grouping keys by ndistinct, ORDER BY or
whatever. In particular I don't think we should commit these patches
that may quite easily cause regressions, and then hope some hypothetical
future patch fixes the costing. Those objections still stand.


Pls, have a look at https://commitfest.postgresql.org/18/1706/
I tried to attack the cost_sort() issues and hope on that basis we can solve 
problems with 0002 patch and improve incremental sort patch.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: Server crashed with "TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)"

[Rajkumar Raghuwanshi]

On Thu, Jun 28, 2018 at 9:45 PM, Andres Freund  wrote:

> Hi,
>
> On 2018-06-28 16:08:31 +0530, Rajkumar Raghuwanshi wrote:
> > I am getting server crash with below test case, logfile message and core
> > dump details in the mail.
>
> Which versions are affected here? Is this reproducible in postgres 10 or
> just the current beta?
>

This affected current beta only as this is not reproducible in postgres 10
because index create on
partitioned table is not supported in v10.

Re: Fix to not check included columns in ANALYZE on indexes

[Tom Lane]

Andres Freund  writes:
> On June 28, 2018 4:28:39 PM PDT, Tom Lane  wrote:
>> (In principle, CREATE STATISTICS might someday obsolete this use-case
>> for expression indexes, but it hasn't done so yet AFAIK.)

> You mean stats on them, or the feature entirely? If the latter, how?

No, just the collection-of-stats-on-an-expression part.

regards, tom lane

Re: partition tree inspection functions

[Amit Langote]

On 2018/06/28 22:01, Ashutosh Bapat wrote:
> On Thu, Jun 28, 2018 at 11:19 AM, Amit Langote
>  wrote:
>>>
>>> It would have imagined that just creating a new file, say
>>> partition_desc.sql or similar is nicer.
>>
>> How about partition_info.sql because they're testing partitioning
>> information functions?  partition_desc reminded me of PartitionDesc, an
>> internal structure used in the partitioning codem which made me a bit
>> uncomfortable.
> 
> I think we should just add calls to these functions/views wherever we
> are creating/altering or deleting objects to test a partition tree. I
> serves two purposes, testing the objects created/modified and testing
> the functions. Adding a new test file means we have to craft new
> objects, which are sometimes readily available in some other test
> files. Of course, we might find that certain cases are not covered by
> existing tests, but then that also means that those cases are not
> covered by object modification/creation tests as well.

I think that makes sense.  I couldn't assess by looking at tests for
various functions listed on 9.25. System Information Functions whether
there is some established practice about adding tests for them and/or
about where to put them.

For this particular set of functions, insert.sql may be a good place as it
has many tests involving multi-level partitioned tables.

Thanks,
Amit

Re: Generating partitioning tuple conversion maps faster

[David Rowley]

On 29 June 2018 at 11:10, David Rowley  wrote:
> On further inspection, the slowdown is coming from the planner in
> make_inh_translation_list(). The INSERT path does not hit that since
> the planner's job is pretty simple for simple INSERTs.

I've attached a patch that uses SearchSysCacheAttName to speed up
these translations in the planner.

This puts test 6 more at the level I'd have expected.

Here are fresh benchmarks results taken with the attached, again on
AWS m5d instance, though probably not the same one as before
(fsync=off).


Unpatched:

Test 1
tps = 922.479156 (excluding connections establishing)

Test 2
tps = 334.701555 (excluding connections establishing)

Test 3
tps = 327.547316 (excluding connections establishing)

Test 4
tps = 1198.924131 (excluding connections establishing)

Test 5
tps = 125.130723 (excluding connections establishing)

Test 6
tps = 81.511072 (excluding connections establishing)

Patched

Test 1
tps = 918.105382 (excluding connections establishing)

Test 2
tps = 913.315387 (excluding connections establishing)

Test 3
tps = 893.578988 (excluding connections establishing)

Test 4
tps = 1213.238177 (excluding connections establishing)

Test 5
tps = 459.022550 (excluding connections establishing)

Test 6
tps = 416.835747 (excluding connections establishing)


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


v2_speedup_building_tuple_conversion_maps.patch
Description: Binary data

Re: Capitalization of the name OpenSSL

[Michael Paquier]

On Thu, Jun 28, 2018 at 09:54:35AM +0200, Daniel Gustafsson wrote:
> On 28 Jun 2018, at 04:54, Michael Paquier  wrote:
>> I see more that 860 references to openssl as lower case with lots of
>> noise around file names and translations.  And I can see that you missed
>> two of them: 
> 
> Nice catch, fixed in the attached.

I found a couple of extra ones in the past release notes, which I fixed
as well to make all references more consistent.

>> I don't think that the comments are worth bothering for anything else
>> than HEAD, now should the doc changes be back-patched?  I would tend to
> > do so.
>
> Not sure if it’s worth any back-patching at all to be honest, but I’ll leave
> that call to you.

Yes, done only on HEAD.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, Jun 29, 2018 at 10:37:55AM +0900, Michael Paquier wrote:
> The set of APIs that we use to the SSL abstraction layer is very
> internal, so it would not be an issue if we add some in stable branches,
> no?  My point is that from OpenSSL point of view, TLS 1.3 stuff has been
> added in 1.1.1 which is now in beta 6 stage, so we could consider as
> well all this part once OpenSSL is released.  That's compatibility work
> I wanted to work on anyway.  Impossible to say down to which versions of
> Postgres things could be applied easily though without a deep
> investigation of the new compatibility breakages that upstream OpenSSL
> has very-likely introduced in upstream.
> 
> Still it does not sound completely strange either to me to wait for
> OpenSSL to release as we won't be able to have a full solution designed
> before that.

Actually, I got curious about that part and just compiled Postgres with
OpenSSL 1.1.1 beta 6 that I compiled manually, and channel binding is
generating consistent data for both tls-unique and tls-server-end-point
even if TLS v1.3 is used, while tests in src/test/ssl/ are all able to
pass.  So that's less dramatic than what I thought after the melodrama
of upgrading the code to 1.1.0.

The thread where this is discussed is also kind of interesting as the
last email points to having tls-unique deprecated for all the TLS
versions:
https://www.ietf.org/mail-archive/web/tls/current/msg18265.html

I am able to find easily drafts of TLS 1.3, but I am not seeing an RFC
associated to it, which would be the base document to rely on I
guess...  So that's really hard to make any decision in this area
without the real deal.  As far as I can see tls-unique could be
deprecated and replaced, but from OpenSSL point of view it technically
works.
--
Michael


signature.asc
Description: PGP signature

Re: Fix to not check included columns in ANALYZE on indexes

[Andres Freund]

On June 28, 2018 4:28:39 PM PDT, Tom Lane  wrote:
>Andres Freund  writes:
>> On June 28, 2018 4:18:36 PM PDT, Tom Lane  wrote:
>>> Also, is it really true that we don't support included expression
>>> columns now?  In what way would that not be a bug?
>
>> I don't think IOS supports expression columns, right?  Away from code
>for a bit, so can't check.  If indeed true, there'd be little point in
>allowing it, right?
>
>The point of ANALYZE on an expression column is that you can direct
>ANALYZE to collect stats on that expression.  This is potentially
>valuable
>for rowcount estimation whether or not the planner notices that it can
>fetch the expression value from the index, or chooses to do so even if
>it
>did notice.

The whole point of including additional columns in the index is that they allow 
IOSs. It seems more likely that people will expect that included expressions 
actually are usable, than enlarging the index just to get a bit better stats.


>(In principle, CREATE STATISTICS might someday obsolete this use-case
>for expression indexes, but it hasn't done so yet AFAIK.)

You mean stats on them, or the feature entirely? If the latter, how?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Thu, Jun 28, 2018 at 10:05:23AM +0200, Peter Eisentraut wrote:
> But before we drop the SCRAM business completely off the open items, I
> think we need to consider how TLS 1.3 affects this.

The set of APIs that we use to the SSL abstraction layer is very
internal, so it would not be an issue if we add some in stable branches,
no?  My point is that from OpenSSL point of view, TLS 1.3 stuff has been
added in 1.1.1 which is now in beta 6 stage, so we could consider as
well all this part once OpenSSL is released.  That's compatibility work
I wanted to work on anyway.  Impossible to say down to which versions of
Postgres things could be applied easily though without a deep
investigation of the new compatibility breakages that upstream OpenSSL
has very-likely introduced in upstream.

Still it does not sound completely strange either to me to wait for
OpenSSL to release as we won't be able to have a full solution designed
before that.
--
Michael


signature.asc
Description: PGP signature

Re: Does logical replication supports cross platform servers?

[Bruce Momjian]

On Wed, Jun 27, 2018 at 11:16:07PM +1000, Haribabu Kommi wrote:
> On Sun, Jun 24, 2018 at 6:36 AM Bruce Momjian  wrote:
> 
> On Wed, Jun 13, 2018 at 11:42:14AM +1000, Haribabu Kommi wrote:
> >
> > Thanks for the confirmation.
> > This is a good use case that must be explicitly mentioned in the docs.
> > How about the attached patch?
> 
> OK, doc patch added to git head.  Let me know if I should back patch it
> further.  Thanks.
> 
>  
> Thanks for the commit. 
> Yes, it needs to be back patched till 10 stable.

Sure, done, thanks.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: cost_sort() improvements

[Peter Geoghegan]

On Thu, Jun 28, 2018 at 9:47 AM, Teodor Sigaev  wrote:
> Current estimation of sort cost has following issues:
>  - it doesn't differ one and many columns sort
>  - it doesn't pay attention to comparison function cost and column width
>  - it doesn't try to count number of calls of comparison function on per
> column
>basis

I've been suspicious of the arbitrary way in which I/O for external
sorts is costed by cost_sort() for a long time. I'm not 100% sure
about how we should think about this question, but I am sure that it
needs to be improved in *some* way. It's really not difficult to show
that external sorts are now often faster than internal sorts, because
they're able to be completed on-the-fly, which can have very good CPU
cache characteristics, and because the I/O latency can be hidden
fairly well much of the time. Of course, as memory is taken away,
external sorts will eventually get slower and slower, but it's
surprising how little difference it makes. (This makes me tempted to
look into a sort_mem GUC, even though I suspect that that will be
controversial.)

Clearly there is a cost to doing I/O even when an external sort is
faster than an internal sort "in isolation"; I/O does not magically
become something that we don't have to worry about. However, the I/O
cost seems more and more like a distributed cost. We don't really have
a way of thinking about that at all. I'm not sure if that much bigger
problem needs to be addressed before this specific problem with
cost_sort() can be addressed.

-- 
Peter Geoghegan

Re: Forbid referencing columns by names in ALTER INDEX ... SET STATISTICS

[Robert Haas]

On Wed, Jun 27, 2018 at 9:22 AM, Yugo Nagata  wrote:
> According to the syntax in ALTER INDEX doc, a column should be specified by
> column number as discussed in [1]. However, the current code still allows to
> use an internal column name like "expr". Is this intentional?
>
> Although it is harmless, how about forbiding this undocumented and
> unuseful behavior. The attached patch does it.

If it's harmless, why prohibit it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Tips on committing

[Bruce Momjian]

On Thu, Jun 28, 2018 at 11:14:38AM -0400, Alvaro Herrera wrote:
> I'm not sure pgsql-committers was the right audience. Cross-posting to
> pg-hackers.
> 
> On 2018-Jun-28, Bruce Momjian wrote:
> 
> >  2  Reported-by:
> >  5  Author:
> >  6  Reviewed-by:
> >  7  Tested-by:
> 
> Should these include email addresses?

Uh, since I am linking to the thread it seemed unnecessary.

> I've also used "Diagnosed-by" to credit a person who spent time studying
> a bug's mechanism and how to fix it.  Sometimes that's the same as
> Reported-by, but I think the weight is quite different.

Good point.  I have never used it but I can see its value.  I have added
it to my template.

> Apparently, there's a recent trend to credit patch authors using
> "Co-authored-by".  Should we use that too?
> https://stackoverflow.com/a/41847267/

I would just list multiple authors.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Tips on committing

[Peter Geoghegan]

On Thu, Jun 28, 2018 at 9:52 AM, Alvaro Herrera
 wrote:
>> FWIW, I developed a document on committing for my own reference, with
>> some help from Andres.
>
> Sounds very useful.
>
> How about turning it into a wiki page, for everybody's benefit?

I'll try to do that, but I'd still recommend personalizing it. A lot
of the stuff in there is specific to my own workflow and tool
preferences, and my own personal working style. I don't really use a
template in the same way Bruce does, for example, because most of that
stuff is taken care of by my text editor having a "gitcommit" syntax.

I find it useful to have a routine checklist for things like
committing, because it frees up a little space in my head for other
things. I do the same thing when preparing for a trip.

-- 
Peter Geoghegan

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Peter Eisentraut]

On 6/28/18 22:52, Alvaro Herrera wrote:
>> Couldn't psql chase the pg_depend links to find inherited triggers?
> 
> Yeah, I thought this would be exceedingly ugly, but apparently it's not
> *that* bad -- see the attached patch, which relies on the fact that
> triggers don't otherwise depend on other triggers.  We don't use this
> technique elsewhere in psql though.

Yeah, relying on pg_depend to detect relationships between catalog
objects is a bit evil.  We do use this for detecting sequences linked to
tables, which also has its share of problems.  Ideally, there would be a
column in pg_trigger, perhaps, that makes this link explicit.  But we
are looking here for a solution without catalog changes, I believe.

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

Re: partition tree inspection functions

[Peter Eisentraut]

On 6/28/18 13:30, Michael Paquier wrote:
> On Thu, Jun 28, 2018 at 12:37:14PM +0200, Peter Eisentraut wrote:
>> I'm thinking, an SQL query might be more efficient if you want to
>> qualify the query further.  For example, give me all tables in this tree
>> that match '2018'.  If you wrote your functions as SQL-language
>> functions, the optimizer could perhaps inline them and optimize them
>> further.
> 
> Are you thinking about SQL functions here?  Here is an example of query
> able to fetch an entire partition tree.
> WITH RECURSIVE partition_info
>   (relid,
>relname,
>relsize,
>relispartition,
>relkind) AS (
> SELECT oid AS relid,
>relname,
>pg_relation_size(oid) AS relsize,
>relispartition,
>relkind
> FROM pg_catalog.pg_class
>   WHERE relname = 'your_parent_table_name_here' AND
> relkind = 'p'
[...]

Yes, this kind of thing should be more efficient than building the
entire tree in a C function and then filtering it afterwards.

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

Re: SQL/JSON: documentation

[Nikita Glukhov]

On 28.06.2018 05:23, Chapman Flack wrote:


On 06/27/2018 07:36 PM, Nikita Glukhov wrote:


Also it can be found in our sqljson repository on sqljson_doc branch:
https://github.com/postgrespro/sqljson/tree/sqljson_doc

Perhaps it is my unfamiliarity, but it seems that on lines 1067–1071,
the output clause for JSON_VALUE is given support for return types
json, jsonb, bytea, text, char, varchar, nchar "out of the box".

There are then examples on lines 1123–1135 of returning float, int,
and date.

Does that mean that the list in 1067–1071 is incomplete, and should
include additional data types?

Or does it mean that there is more cleverness buried in the
"must ... have a cast to the specified type" language than I
first understood?

Does the function support returning some wanted type w, not in the
out-of-the-box list, such as float, by searching for an intermediate
type t ∈ {json, jsonb, bytea, text, char, varchar, nchar} such that
∃ cast(t as w), then representing the JSON value as t, then casting
that to w ?

If so, what does it do if more than one t is a candidate?


First, thank you for your interest in SQL/JSON docs.


Standard says only about returning of string (both binary and character),
numeric, boolean and datetime types in JSON_VALUE and only about string
types in JSON_QUERY.

In JSON_VALUE first searched cast from the SQL type corresponding to the
SQL/JSON type of a resulting scalar item to the target RETURNING type.

SQL/JSON typePG SQL type
string=> text
number=> numeric
boolean   => boolean
date  => date
time  => time
time with tz  => timetz
timestamp => timestamp
timestamp with tz => timestamptz

If this cast does not exist then conversion via input/output is tried (this
is our extension).  But json and jsonb RETURNING types are exceptional here,
because SQL/JSON items can be converted directly to json[b] without casting.

But we also support returning of arbitrary PG types including arrays, domains
and records in both JSON_VALUE and JSON_QUERY.  In JSON_VALUE values of this
types should be represented as serialized JSON strings, because JSON_VALUE
supports only returning of scalar items.  The behavior of JSON_QUERY is similar
to the behavior json[b]_populate_record().

Examples:

-- CAST(numeric AS int) is used here
=# SELECT JSON_VALUE('1.8', '$' RETURNING int);
 json_value

  2
(1 row)

-- CAST(text AS int) is used here
=# SELECT JSON_VALUE('"1"', '$' RETURNING int);
 json_value

  1
(1 row)

-- CAST(text AS int) is used here
=# SELECT JSON_VALUE('"1.8"', '$' RETURNING int ERROR ON ERROR);
ERROR:  invalid input syntax for integer: "1.8"

-- CAST(numeric AS int) is used here
# SELECT JSON_VALUE('"1.8"', '$.double().floor()' RETURNING int);
 json_value

  1
(1 row)


-- array of points serialized into single JSON string
-- CAST(text AS point[]) is used
=# SELECT JSON_VALUE('"{\"(1,2)\",\"3,4\",NULL}"', '$' RETURNING point[]);
   json_value

 {"(1,2)","(3,4)",NULL}
(1 row)

-- point[] is represented by JSON array of point strings
-- ARRAY[CAST(text AS point)] is used
=# SELECT JSON_QUERY('["(1, 2)", " 3 , 4 ", null]', '$' RETURNING point[]);

   json_query

 {"(1,2)","(3,4)",NULL}
(1 row)

-- JSON object converted into SQL record type
=# SELECT JSON_QUERY('{"relname": "foo", "relnatts" : 5}', '$' RETURNING 
pg_class);
   json_query

 (foo5)
(1 row)



Line 2081: "A typical path expression has the following structure"

It seems like a "weasel word" to have "typical" in the statement
of an expression grammar. Is there more to the grammar than is
given here?


Yes, that expression grammar is incomplete because arithmetic operations
are supported on the top of jsonpath accessor expressions.

Here is nearly complete expression grammar (predicates are not included):

jsonpath ::=
  [STRICT | LAX] jsonpath_expression

jsonpath_expression ::=
  jsonpath_additive_expression

jsonpath_additive_expression ::=
  [ jsonpath_additive_expression { + | - } ]
jsonpath_multiplicative_expression

jsonpath_multiplicative_expression ::=
  [ jsonpath_multiplicative_expression  { * | / | % } ]
jsonpath_unary_expression

jsonpath_unary_expression ::=
  jsonpath_accessor_expression
  | { + | - } jsonpath_unary_expression

jsonpath_accessor_expression ::=
  jsonpath_primary  { jsonpath_accessor }[...]

jsonpath_accessor ::=
    . *
  | . key_name
  | . method_name ( jsonpath_expression [, ...] )
  | '[' * ']'
  | '[' jsonpath_expression [, ...] ']'
  |  ? ( predicate )

jsonpath_primary ::=
$
  | @
  | variable
  | literal
  | ( jsonpath_expression )
 


Lines 2323 and 2330 ( / and % operators ). Do these behave differently
for integer than for float operands? If they provide integer operations,
which results do they produce for 

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[Daniel Gustafsson]

> On 27 Jun 2018, at 18:02, Yugo Nagata  wrote:

> I found that there isn't explanation about EXCLUDING in CREATE TABLE doc.
> Attached is a patch to add this. I would appreciate it if a native English
> speaker comments on this.

+  If EXCLUDING option  is specified

The empty  seems wrong.

+  after INCLUDING options, the specified thing is 
excluded

“thing” sounds a bit vague here (and in the last sentence as well), but I’m
also not sure what to use instead.  “referenced objects" perhaps?

+1 on documenting the EXCLUDING option though.

cheers ./daniel

Re: Server crashed with "TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)"

[Alvaro Herrera]

On 2018-Jun-28, Andres Freund wrote:

> On 2018-06-28 22:35:11 +0530, Rajkumar Raghuwanshi wrote:
> > On Thu, Jun 28, 2018 at 9:45 PM, Andres Freund  wrote:
> > > On 2018-06-28 16:08:31 +0530, Rajkumar Raghuwanshi wrote:
> > > > I am getting server crash with below test case, logfile message and core
> > > > dump details in the mail.

> Alvaro, sounds like it could be an open item assigned to you?

You're right.  The attached patch seems to fix it.

I think I should add Rajkumar's test case too, though, so I'm not
pushing this evening.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 8743ee29a66e8ec65db9a1b44fe545088f2b95f2 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Thu, 28 Jun 2018 18:57:05 -0400
Subject: [PATCH] Fix crash when ALTER TABLE recreates indexes on partitions

The skip_build flag was not being passed correctly when recursing to
indexes on partitions, leading to attempts to rebuild indexes when they
were not yet ready to be rebuilt.

Reported-by: Rajkumar Raghuwanshi
Discussion: 
https://postgr.es/m/cakcux6mxncgsgatwf5cgmf8g4wsupcxiccvmekutuwbyxho...@mail.gmail.com
---
 src/backend/commands/indexcmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3b82876c90..576c85f732 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1033,7 +1033,7 @@ DefineIndex(Oid relationId,

indexRelationId,/* this is our child */

createdConstraintId,
is_alter_table, 
check_rights, check_not_in_use,
-   false, quiet);
+   skip_build, 
quiet);
}
 
pfree(attmap);
-- 
2.11.0

Re: Generating partitioning tuple conversion maps faster

[David Rowley]

On 29 June 2018 at 02:23, Alexander Kuzmenkov
 wrote:
> I think I found one possible reason for this slowness. Your patch behaves as
> expected when there is a dropped output column, but does one extra
> comparison when there is a dropped input column. This backwards conversion
> is called from ExecInitRoutingInfo. To fix this, I'd just keep a persistent
> inner loop counter (see the attached patch).

It's just 2000 comparisons vs 1000.

> Still, fixing this doesn't improve the performance. According to perf
> report, updatepd.sql spends 25% of time in strcmp, and updatep.sql about
> 0.5%, but they are comparing the same column names, even with the same
> alignment and relative offsets. I'm completely puzzled by this.

On further inspection, the slowdown is coming from the planner in
make_inh_translation_list(). The INSERT path does not hit that since
the planner's job is pretty simple for simple INSERTs.

> As a side thought, we wouldn't have to go through this if we had a hash
> table that is easy to use, or perhaps string interning in catcache.

Maybe it's better to try the direct lookup and fall back on
SearchSysCacheAttName() if the same attnum does not have the same
name.


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

Re: SQL/JSON: documentation

[Chapman Flack]

On 06/28/2018 06:45 PM, Nikita Glukhov wrote:

> Standard says only about returning of string (both binary and character),
> numeric, boolean and datetime types in JSON_VALUE and only about string
> types in JSON_QUERY.

What I think I noticed was that right now, in func-sqljson.sgml,
the same list of seven types (not including numeric, boolean, or
datetime) is repeated for both JSON_QUERY and JSON_VALUE. Should
the list for JSON_VALUE also mention that numeric, boolean, and
datetime are supported there? That's the description that is near
line 1067.


> Arithmetic operations in jsonpath are implemented using PG numeric
> datatype,
> which also is used in jsonb for representation of JSON numbers:
> ...
> =# SELECT jsonb '-3.4' @* '$ % 2.3';
>  ?column?
> --
>  -1.1

In a recent message[1] it seemed that PG itself relies on the
underlying C compiler behavior, at least for int and float, which
could mean that on some platforms the answer is -1.1 and on others
+1.2. But I don't know whether that is true for PG numeric, since
that is implemented much more within PG itself, so perhaps it has
a platform-independent behavior. The XQuery result would be -1.1 on
all platforms, because the standard is explicit there.

-Chap

[1]: https://www.postgresql.org/message-id/23660.1530070402%40sss.pgh.pa.us

Re: Fix to not check included columns in ANALYZE on indexes

[Tom Lane]

Yugo Nagata  writes:
> I found that both key columns and included columns are checked when analyze 
> is run on indexes. This is almost harmless because non-expression columns
> are not processed. However, this check is obviously unnecessary and we
> can fix this to not check included columns. If we decide to support 
> expressions
> in included columns in future, this must be fixed eventually.

AFAICS, we'd just have to revert this patch later, so I don't see
much value in it.

Also, is it really true that we don't support included expression
columns now?  In what way would that not be a bug?

regards, tom lane

Re: Fix to not check included columns in ANALYZE on indexes

[Andres Freund]

On June 28, 2018 4:18:36 PM PDT, Tom Lane  wrote:
>Yugo Nagata  writes:
>> I found that both key columns and included columns are checked when
>analyze 
>> is run on indexes. This is almost harmless because non-expression
>columns
>> are not processed. However, this check is obviously unnecessary and
>we
>> can fix this to not check included columns. If we decide to support
>expressions
>> in included columns in future, this must be fixed eventually.
>
>AFAICS, we'd just have to revert this patch later, so I don't see
>much value in it.
>
>Also, is it really true that we don't support included expression
>columns now?  In what way would that not be a bug?

I don't think IOS supports expression columns, right?  Away from code for a 
bit, so can't check.  If indeed true, there'd be little point in allowing it, 
right?

Andres

Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: CREATE TABLE .. LIKE .. EXCLUDING documentation

[David G. Johnston]

On Thu, Jun 28, 2018 at 3:57 PM, Daniel Gustafsson  wrote:

> > On 27 Jun 2018, at 18:02, Yugo Nagata  wrote:
>
> > I found that there isn't explanation about EXCLUDING in CREATE TABLE doc.
> > Attached is a patch to add this. I would appreciate it if a native
> English
> > speaker comments on this.
>
> +  If EXCLUDING option  is
> specified
>
> The empty  seems wrong.
>
> +  after INCLUDING options, the specified thing is
> excluded
>
> “thing” sounds a bit vague here (and in the last sentence as well), but I’m
> also not sure what to use instead.  “referenced objects" perhaps?
>
> +1 on documenting the EXCLUDING option though.
>

​"is excluded" and "not copied" are redundant to each other and the first
sentence is basically redundant with the second.

​Maybe try something like:

It is legal to specify the same option multiple times - e.g., "INCLUDING
option EXCLUDING option" - the outcome is whichever comes last in the
command (i.e., in the example, option is excluded).

David J.

Re: alter index WITH ( storage_parameter = value [, ... ] ) for partition index.

[Michael Paquier]

On Thu, Jun 28, 2018 at 11:51:23AM +0530, Rajkumar Raghuwanshi wrote:
> postgres=# select pgstatindex('part_pk');
> ERROR:  relation "part_pk" is not a btree index

This error message is intentional.  Please see bef5fcc and its related
thread:
https://www.postgresql.org/message-id/cah2-wzkokptqie51bh4_xeehhabwhkzkgtkizrfmgekfuur...@mail.gmail.com
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] SERIALIZABLE with parallel query

[Masahiko Sawada]

On Fri, Mar 30, 2018 at 2:56 PM, Thomas Munro
 wrote:
> On Thu, Mar 8, 2018 at 10:28 AM, Robert Haas  wrote:
>> +SerializableXactHandle
>> +ShareSerializableXact(void)
>> +{
>> +Assert(!IsParallelWorker());
>> +
>> +return MySerializableXact;
>> +}
>>
>> Uh, how's that OK?  There's no rule that you can't create a
>> ParallelContext in a worker.  Parallel query currently doesn't, so it
>> probably won't happen, but burying an assertion to that effect in the
>> predicate locking code doesn't seem nice.
>
> Hmm.  I suppose you could have a PARALLEL SAFE function that itself
> launches parallel workers explicitly (not via parallel query), and
> they should inherit the same SERIALIZABLEXACT from their parent and
> that should all just work.
>
>> Is "sxact" really the best (i.e. clearest) name we can come up with
>> for the lock tranche?
>
> Yeah, needs a better name.
>
> I have some lingering uncertainty about this patch and we're out of
> time, so I moved it to PG12 CF1.  Thanks Haribabu, Robert, Amit for
> the reviews and comments so far.
>

I'd like to test and review this patches but they seem to conflict
with current HEAD. Could you please rebase them?

Regards,

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

Re: Capitalization of the name OpenSSL

[Michael Paquier]

On Thu, Jun 28, 2018 at 09:44:13AM +0200, Magnus Hagander wrote:
> Normally I'm the biggest promoter for back-patching documentation changes
> :) But in the case when it's really just about the capitalization, I think
> it's fine to just bother with HEAD for it.

Yeah, after second-thoughts I think that I'll just fix that on HEAD
tomorrow and call it a day if there are no objections after
double-checking that no spots are missing as that's mainly cosmetic.
--
Michael


signature.asc
Description: PGP signature

Re: adding tab completions

[Arthur Zakirov]

Thank you for the new version.

On Wed, Jun 27, 2018 at 03:10:51PM -0500, Justin Pryzby wrote:
> Thanks - I've done this in the attached.  It works well for having minimal
> logic.

I think everthing is OK. But I didn't notice in previous version of the
patch that there is no braces in EXPLAIN and VACUUM conditions. I attached
diff file to show it.

Also I included TEXT, XML, JSON, YAML items for FORMAT option in the
diff file. The patch shows ",", ")", "ON", "OFF" for all options, but
FORMAT requires format type to be specified. Please consider this change
only as a suggestion.

> Thanks for your repeated reviews ; if this looks+works fine, please set to
> R-F-C.
> 
> Actually..another thought: since toast tables may be VACUUM-ed, should I
> introduce Query_for_list_of_tpmt ?

Unfortunately, I'm not sure about toast tables and I'm not aware about
policy of completion toast tables.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index e913b83091..1d235a3987 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2998,7 +2998,7 @@ psql_completion(const char *text, int start, int end)
else if (Matches1("EXECUTE"))
COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
 
-/* 
+/*
  * EXPLAIN [ ( option [, ...] ) ] statement
  * EXPLAIN [ ANALYZE ] [ VERBOSE ] statement
  */
@@ -3006,10 +3006,16 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_LIST8("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE",
"ANALYZE", "VERBOSE", 
"(");
else if (HeadMatches2("EXPLAIN", "("))
+   {
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
COMPLETE_WITH_LIST7("ANALYZE", "VERBOSE", "COSTS", 
"BUFFERS", "TIMING", "SUMMARY", "FORMAT");
-   else
+   else if (TailMatches1("FORMAT"))
+   COMPLETE_WITH_LIST4("TEXT", "XML", "JSON", "YAML");
+   else if 
(TailMatches1("ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
COMPLETE_WITH_LIST4(",", ")", "ON", "OFF");
+   else
+   COMPLETE_WITH_LIST2(",", ")");
+   }
else if (HeadMatches2("EXPLAIN", MatchAny) && ends_with(prev_wd, ')'))
/* If the parenthesis are balanced, the list is apparently 
parsed as a single word */
COMPLETE_WITH_LIST5("SELECT", "INSERT", "DELETE", "UPDATE", 
"DECLARE");
@@ -3597,10 +3603,12 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tpm,
   " UNION 
SELECT 'ANALYZE'");
else if (HeadMatches2("VACUUM", "("))
+   {
if (ends_with(prev_wd, ',') || ends_with(prev_wd, '('))
COMPLETE_WITH_LIST5("FULL", "FREEZE", "ANALYZE", 
"VERBOSE", "DISABLE_PAGE_SKIPPING");
else
COMPLETE_WITH_LIST2(",", ")");
+   }
else if (HeadMatches1("VACUUM") && TailMatches1("("))
/* "VACUUM (" should be caught above */
COMPLETE_WITH_ATTR(prev2_wd, "");

Server crashed with "TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)"

[Rajkumar Raghuwanshi]

Hi,

I am getting server crash with below test case, logfile message and core
dump details in the mail.

postgres=# CREATE TABLE part_tbl (a INT, b TEXT, c INT) PARTITION BY
RANGE(a);
CREATE TABLE
postgres=# CREATE TABLE part_tbl_p PARTITION OF part_tbl FOR VALUES FROM
(minvalue) TO (maxvalue);
CREATE TABLE
postgres=# CREATE INDEX partidx_abc_idx ON part_tbl (a, b, c);
CREATE INDEX
postgres=# INSERT INTO part_tbl (a, b, c) SELECT i,i,i FROM
generate_series(1,50)i;
INSERT 0 50
postgres=# ANALYZE part_tbl;
ANALYZE
postgres=# ALTER TABLE part_tbl ALTER COLUMN c TYPE numeric;
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.
!>


--logfile
TRAP: unrecognized TOAST vartag("1", File: "heaptuple.c", Line: 1490)
2018-06-28 00:01:01.679 IST [97062] LOG:  server process (PID 97080) was
terminated by signal 6: Aborted
2018-06-28 00:01:01.679 IST [97062] DETAIL:  Failed process was running:
ALTER TABLE part_tbl ALTER COLUMN c TYPE numeric;

--corefile
Core was generated by `postgres: edb postgres [local] ALTER
TABLE  '.
Program terminated with signal 6, Aborted.
#0  0x003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-23.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-57.el6.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0  0x003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x003dd2633c75 in abort () at abort.c:92
#2  0x00a337ee in ExceptionalCondition (conditionName=0xaa3290 "1",
errorType=0xaa3276 "unrecognized TOAST vartag", fileName=0xaa31e0
"heaptuple.c", lineNumber=1490)
at assert.c:54
#3  0x00487c9c in slot_deform_tuple (slot=0x1fa2d68, natts=3) at
heaptuple.c:1490
#4  0x00487fc2 in slot_getattr (slot=0x1fa2d68, attnum=3,
isnull=0x7ffea43368e7) at heaptuple.c:1595
#5  0x0057b10d in FormIndexDatum (indexInfo=0x1f91138,
slot=0x1fa2d68, estate=0x1f95c98, values=0x7ffea4336be0,
isnull=0x7ffea4336bc0) at index.c:2032
#6  0x0057c459 in IndexBuildHeapRangeScan
(heapRelation=0x7f3944067c40, indexRelation=0x7f3944067a28,
indexInfo=0x1f91138, allow_sync=true, anyvisible=false,
start_blockno=0, numblocks=4294967295, callback=0x5123ab
<_bt_build_callback>, callback_state=0x7ffea4336ea0, scan=0x1f91b80) at
index.c:2876
#7  0x0057bac1 in IndexBuildHeapScan (heapRelation=0x7f3944067c40,
indexRelation=0x7f3944067a28, indexInfo=0x1f91138, allow_sync=true,
callback=0x5123ab <_bt_build_callback>, callback_state=0x7ffea4336ea0,
scan=0x0) at index.c:2426
#8  0x0051 in _bt_spools_heapscan (heap=0x7f3944067c40,
index=0x7f3944067a28, buildstate=0x7ffea4336ea0, indexInfo=0x1f91138) at
nbtsort.c:472
#9  0x00511f72 in btbuild (heap=0x7f3944067c40,
index=0x7f3944067a28, indexInfo=0x1f91138) at nbtsort.c:324
#10 0x0057b6fb in index_build (heapRelation=0x7f3944067c40,
indexRelation=0x7f3944067a28, indexInfo=0x1f91138, isprimary=false,
isreindex=false, parallel=true)
at index.c:2288
#11 0x00579d49 in index_create (heapRelation=0x7f3944067c40,
indexRelationName=0x1f9b370 "part_tbl_p_a_b_c_idx", indexRelationId=16396,
parentIndexRelid=16395,
parentConstraintId=0, relFileNode=0, indexInfo=0x1f91138,
indexColNames=0x1f9b288, accessMethodObjectId=403, tableSpaceId=1663,
collationObjectId=0x1f9b460,
classObjectId=0x1f9b488, coloptions=0x1f9b4b0, reloptions=0, flags=0,
constr_flags=0, allow_system_table_mods=false, is_internal=true,
constraintId=0x7ffea433715c)
at index.c:1187
#12 0x0066488d in DefineIndex (relationId=16387, stmt=0x1f90eb8,
indexRelationId=0, parentIndexId=16395, parentConstraintId=0,
is_alter_table=true, check_rights=false,
check_not_in_use=false, skip_build=false, quiet=true) at indexcmds.c:850
#13 0x00664e7f in DefineIndex (relationId=16384, stmt=0x1f79688,
indexRelationId=16395, parentIndexId=0, parentConstraintId=0,
is_alter_table=true, check_rights=false,
check_not_in_use=false, skip_build=true, quiet=true) at indexcmds.c:1031
#14 0x0068ff68 in ATExecAddIndex (tab=0x1f744f8,
rel=0x7f3944061b50, stmt=0x1f79688, is_rebuild=true, lockmode=8) at
tablecmds.c:6958
#15 0x0068977f in ATExecCmd (wqueue=0x7ffea4337838, tab=0x1f744f8,
rel=0x7f3944061b50, cmd=0x1f90e60, lockmode=8) at tablecmds.c:4090
#16 0x006891a9 in ATRewriteCatalogs (wqueue=0x7ffea4337838,
lockmode=8) at tablecmds.c:3989
#17 0x0060 in ATController (parsetree=0x1f81460,
rel=0x7f3944061b50, cmds=0x1f74460, recurse=true, lockmode=8) at
tablecmds.c:3639
#18 0x006885a9 in AlterTable (relid=16384, lockmode=8,
stmt=0x1f81460) at tablecmds.c:3312
#19 0x008cea8e 

Re: partition tree inspection functions

[Peter Eisentraut]

On 6/28/18 10:59, Amit Langote wrote:
> On 2018/06/28 17:40, Peter Eisentraut wrote:
>> On 6/26/18 07:08, Amit Langote wrote:
>>> As discussed a little while back [1] and also recently mentioned [2], here
>>> is a patch that adds a set of functions to inspect the details of a
>>> partition tree.  There are three functions:
>>>
>>> pg_partition_parent(regclass) returns regclass
>>> pg_partition_root_parent(regclass) returns regclass
>>> pg_partition_tree_tables(regclass) returns setof regclass
>>
>> Does this add anything over writing a recursive query on pg_inherits?
> 
> As far as the information output is concerned, it doesn't.

I'm thinking, an SQL query might be more efficient if you want to
qualify the query further.  For example, give me all tables in this tree
that match '2018'.  If you wrote your functions as SQL-language
functions, the optimizer could perhaps inline them and optimize them
further.

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

Re: Unexpected behavior of DROP VIEW/TABLE IF EXISTS

[Isaac Morland]

On 28 June 2018 at 05:37, Peter Moser  wrote:
[]

> In general my use-case is, that I want to delete an X in a certain
> namespace, where the type is not known in advance. I could query the
> catalog to get that information and then build a procedure to "execute" the
> right DROP command (as Pavel Stehule suggested), but that adds complexity
> to the application code, where it shouldn't be necessary IMHO.
>

I've dealt with this issue in some contexts by writing a procedure which
takes a regclass parameter and formats and executes an appropriate "DROP
[x]" command.

On a related note, I sometimes find myself wanting to drop a bunch of
tables and views and I find it inconvenient that I have to split up my drop
into two commands - one for the views and one for the tables.

This is a vote for a "DROP RELATION" command that doesn't care if the
objects are views, materialized views, tables, or a mix of those. Maybe
even index or sequence or the other possible values of pg_class.relkind,
although I don't normally think of those as relations.

Re: partition tree inspection functions

[Michael Paquier]

On Thu, Jun 28, 2018 at 12:37:14PM +0200, Peter Eisentraut wrote:
> I'm thinking, an SQL query might be more efficient if you want to
> qualify the query further.  For example, give me all tables in this tree
> that match '2018'.  If you wrote your functions as SQL-language
> functions, the optimizer could perhaps inline them and optimize them
> further.

Are you thinking about SQL functions here?  Here is an example of query
able to fetch an entire partition tree.
WITH RECURSIVE partition_info
  (relid,
   relname,
   relsize,
   relispartition,
   relkind) AS (
SELECT oid AS relid,
   relname,
   pg_relation_size(oid) AS relsize,
   relispartition,
   relkind
FROM pg_catalog.pg_class
WHERE relname = 'your_parent_table_name_here' AND
  relkind = 'p'
  UNION ALL
SELECT
 c.oid AS relid,
 c.relname AS relname,
 pg_relation_size(c.oid) AS relsize,
 c.relispartition AS relispartition,
 c.relkind AS relkind
FROM partition_info AS p,
 pg_catalog.pg_inherits AS i,
 pg_catalog.pg_class AS c
WHERE p.relid = i.inhparent AND
 c.oid = i.inhrelid AND
 c.relispartition
  )
SELECT * FROM partition_info;

Getting the direct parent is immediate, and getting the top-most parent
would be rather similar to that.  Not much elegant in my opinion, but
that's mainly a matter of taste?
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Thu, Jun 28, 2018 at 10:04:05AM +0200, Peter Eisentraut wrote:
> On 6/28/18 09:35, Magnus Hagander wrote:
> > No, we absolutely still have SCRAM channel binding.
> > 
> > *libpq* has no way to *enforce* it, meaning it always acts like our
> > default SSL config which is "use it if available but if it's not then
> > silently accept the downgrade". From a security perspective, it's just
> > as bad as our default ssl config, but unlike ssl you can't configure a
> > requirement in 11.
> 
> Isn't this similar to what happened whenever we added a new or better
> password method?  A MITM that didn't want to bother cracking MD5 could
> just alter the stream and request "password" authentication.  Same with
> MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
> the SCRAM hashing method.  Clearly, we need a more comprehensive
> solution for this.

The issue is that our different password methods were designed to do a
best-effort at preventing _passive_ snoopers from decrypting or reusing
passwords.  With channel binding, we are trying to prevent _active_
network attacks, and our fallback behavior eliminates the protection
that channel binding provides.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: partition tree inspection functions

[Ashutosh Bapat]

On Thu, Jun 28, 2018 at 11:19 AM, Amit Langote
 wrote:
>>
>> It would have imagined that just creating a new file, say
>> partition_desc.sql or similar is nicer.
>
> How about partition_info.sql because they're testing partitioning
> information functions?  partition_desc reminded me of PartitionDesc, an
> internal structure used in the partitioning codem which made me a bit
> uncomfortable.

I think we should just add calls to these functions/views wherever we
are creating/altering or deleting objects to test a partition tree. I
serves two purposes, testing the objects created/modified and testing
the functions. Adding a new test file means we have to craft new
objects, which are sometimes readily available in some other test
files. Of course, we might find that certain cases are not covered by
existing tests, but then that also means that those cases are not
covered by object modification/creation tests as well.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: alter index WITH ( storage_parameter = value [, ... ] ) for partition index.

[Rajkumar Raghuwanshi]

another case where I got error like partition table index is not a index is
given below.

postgres=# create table part(a int, constraint part_pk primary key(a))
PARTITION BY RANGE(a);
CREATE TABLE
postgres=# create table part_p1 partition of part for values from
(minvalue) to (0);
CREATE TABLE
postgres=# create table part_p2 partition of part for values from (0) to
(maxvalue);
CREATE TABLE
postgres=# create EXTENSION if not exists pgstattuple;
CREATE EXTENSION
postgres=# select pgstatindex('part_p1_pkey');
 pgstatindex
--
 (3,0,8192,0,0,0,0,0,NaN,NaN)
(1 row)

postgres=# select pgstatindex('part_pk');
ERROR:  relation "part_pk" is not a btree index
postgres=#

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

On Wed, Jun 27, 2018 at 3:12 PM, Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> Hi,
>
> I have created partition table index with some storage_parameter like
> example given below, I am not able to reset/modify it from partition table.
> Is this fine.
>
> postgres=# create table part(a int) PARTITION BY RANGE(a);
> CREATE TABLE
> postgres=# create table part_p partition of part for values from
> (minvalue) to (maxvalue);
> CREATE TABLE
> postgres=# create index part_idx on part(a) with (fillfactor = '14');
> CREATE INDEX
> postgres=# \d part
> Table "public.part"
>  Column |  Type   | Collation | Nullable | Default
> +-+---+--+-
>  a  | integer |   |  |
> Partition key: RANGE (a)
> Indexes:
> "part_idx" btree (a) WITH (fillfactor='14')
> Number of partitions: 1 (Use \d+ to list them.)
>
> postgres=# \d part_p
>Table "public.part_p"
>  Column |  Type   | Collation | Nullable | Default
> +-+---+--+-
>  a  | integer |   |  |
> Partition of: part FOR VALUES FROM (MINVALUE) TO (MAXVALUE)
> Indexes:
> "part_p_a_idx" btree (a) WITH (fillfactor='14')
>
>
> *postgres=# alter index part_idx reset (fillfactor);ERROR:  "part_idx" is
> not a table, view, materialized view, or index*
> postgres=# alter index part_p_a_idx reset (fillfactor);
> ALTER INDEX
> postgres=# \d+ part
>Table "public.part"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> +-+---+--+-+
> -+--+-
>  a  | integer |   |  | | plain
> |  |
> Partition key: RANGE (a)
> Indexes:
>* "part_idx" btree (a) WITH (fillfactor='14')*
> Partitions: part_p FOR VALUES FROM (MINVALUE) TO (MAXVALUE)
>
> postgres=# \d part_p
>Table "public.part_p"
>  Column |  Type   | Collation | Nullable | Default
> +-+---+--+-
>  a  | integer |   |  |
> Partition of: part FOR VALUES FROM (MINVALUE) TO (MAXVALUE)
> Indexes:
> "part_p_a_idx" btree (a)
>
> Thanks & Regards,
> Rajkumar Raghuwanshi
> QMG, EnterpriseDB Corporation
>

Re: alter index WITH ( storage_parameter = value [, ... ] ) for partition index.

[Rajkumar Raghuwanshi]

On Thu, Jun 28, 2018 at 12:07 PM, Michael Paquier 
wrote:

> On Thu, Jun 28, 2018 at 11:51:23AM +0530, Rajkumar Raghuwanshi wrote:
> > postgres=# select pgstatindex('part_pk');
> > ERROR:  relation "part_pk" is not a btree index
>
> This error message is intentional.  Please see bef5fcc and its related
> thread:
> https://www.postgresql.org/message-id/CAH2-WzkOKptQiE51Bh4_
> xeehhabwhkzkgtkizrfmgekfuur...@mail.gmail.com


Thanks, Sorry I missed thread.

>
>

Fix to not check included columns in ANALYZE on indexes

[Yugo Nagata]

Hi,

I found that both key columns and included columns are checked when analyze 
is run on indexes. This is almost harmless because non-expression columns
are not processed. However, this check is obviously unnecessary and we
can fix this to not check included columns. If we decide to support expressions
in included columns in future, this must be fixed eventually.

Attached is a patch to fix this.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 25194e8..d2b2c39 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -493,7 +493,7 @@ do_analyze_rel(Relation onerel, int options, VacuumParams *params,
 thisdata->vacattrstats = (VacAttrStats **)
 	palloc(indexInfo->ii_NumIndexAttrs * sizeof(VacAttrStats *));
 tcnt = 0;
-for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
+for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
 {
 	int			keycol = indexInfo->ii_IndexAttrNumbers[i];
 

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Thu, Jun 28, 2018 at 09:35:57AM +0200, Magnus Hagander wrote:
> No, we absolutely still have SCRAM channel binding.
> 
> *libpq* has no way to *enforce* it, meaning it always acts like our default 
> SSL
> config which is "use it if available but if it's not then silently accept the
> downgrade". From a security perspective, it's just as bad as our default ssl
> config, but unlike ssl you can't configure a requirement in 11.

I think we are much more likely to be able to force channel binding by
default since there is no need to configure a certificate authority.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Peter Eisentraut]

On 6/27/18 23:01, Alvaro Herrera wrote:
> Another angle is that we're already in beta3 and there may be concerns
> about altering catalog definition this late in the cycle.  Anybody?
> Maybe psql can just match tgisinternal triggers by name, and if one
> match occurs then we assume it's a clone that should be listed as a
> partition trigger.

Couldn't psql chase the pg_depend links to find inherited triggers?

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

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Thu, Jun 28, 2018 at 10:04 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/28/18 09:35, Magnus Hagander wrote:
> > No, we absolutely still have SCRAM channel binding.
> >
> > *libpq* has no way to *enforce* it, meaning it always acts like our
> > default SSL config which is "use it if available but if it's not then
> > silently accept the downgrade". From a security perspective, it's just
> > as bad as our default ssl config, but unlike ssl you can't configure a
> > requirement in 11.
>
> Isn't this similar to what happened whenever we added a new or better
> password method?  A MITM that didn't want to bother cracking MD5 could
> just alter the stream and request "password" authentication.  Same with
> MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
> the SCRAM hashing method.  Clearly, we need a more comprehensive
> solution for this.
>

That is sort of the gist of the discussion, yes. It is.

So if you just enabled scram channel binding, an attacker could just turn
off scram completely.

That's why we need a solution that covers the full problem, which is why it
needs to be thought of as one problem so we don't end up with a fragmented
solution.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Copy function for logical replication slots

[Peter Eisentraut]

On 6/28/18 08:47, Michael Paquier wrote:
>>> There could be some cases where
>>> copying a physical slot also makes sense.
>> I've thought that but I didn't find concrete use case. That's why I
>> started with only logical slot.
> Let's imagine the case of a single base backup which is associated to a
> given replication slot, and that this backup is then used to spawn
> multiple standbys where each one of them needs a separate slot to
> consume changes at their pace.  If you can copy the slot used in the
> first backup, then both nodes could consume it.  That looks useful to
> me to make sure that both slots are based a consistent point.

I think this use case of cloning replicas would also be interesting in
the logical slot case.

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

Fix error message when trying to alter statistics on included column

[Yugo Nagata]

Hi,

According to the error message, it is not allowed to alter statistics on
included column because this is "non-expression column".

 postgres=# create table test (i int, d int);
 CREATE TABLE
 postgres=# create index idx on test(i) include (d);
 CREATE INDEX
 postgres=# alter index idx alter column 2 set statistics 10;
 ERROR:  cannot alter statistics on non-expression column "d" of index "idx"
 HINT:  Alter statistics on table column instead.

However, I think this should be forbidded in that this is not a key column 
but a included column. Even if we decide to support expressions in included
columns in future, it is meaningless to do this because any statistics on 
included column is never used by the planner.

Attached is the patch to fix the error message. In this fix, column number
is checked first. After applying this, the message is changed as below;

 postgres=# alter index idx alter column 2 set statistics 10;
 ERROR:  cannot alter statistics on included column "d" of index "idx"

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c0cf0d..4beb160 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6504,14 +6504,21 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
  errmsg("cannot alter system column \"%s\"",
 		colName)));
 
-	if ((rel->rd_rel->relkind == RELKIND_INDEX ||
-		 rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) &&
-		rel->rd_index->indkey.values[attnum - 1] != 0)
-		ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot alter statistics on non-expression column \"%s\" of index \"%s\"",
-		NameStr(attrtuple->attname), RelationGetRelationName(rel)),
- errhint("Alter statistics on table column instead.")));
+	if (rel->rd_rel->relkind == RELKIND_INDEX ||
+		 rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+	{
+		if (attnum > rel->rd_index->indnkeyatts)
+			ereport(ERROR,
+	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+	 errmsg("cannot alter statistics on included column \"%s\" of index \"%s\"",
+			NameStr(attrtuple->attname), RelationGetRelationName(rel;
+		else if (rel->rd_index->indkey.values[attnum - 1] != 0)
+			ereport(ERROR,
+	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+	 errmsg("cannot alter statistics on non-expression column \"%s\" of index \"%s\"",
+			NameStr(attrtuple->attname), RelationGetRelationName(rel)),
+	 errhint("Alter statistics on table column instead.")));
+	}
 
 	attrtuple->attstattarget = newtarget;
 

Re: Unexpected behavior of DROP VIEW/TABLE IF EXISTS

[Peter Moser]

On 06/26/2018 07:06 PM, Tom Lane wrote:

Also worth noting is that similar issues arise elsewhere, eg we now
have "procedures" vs "functions" in a single namespace.  Let's not have
DROP TABLE acting in a way that's inconsistent with other parts of the
system.


I think, that

DROP   ...

could search within the type's namespace for the  in 
combination, and only fail if it cannot be found.


I use those commands in a project with an Java ORM in place, that 
automatically generates/updates a schema on each startup. It wrongly 
generates a table X, where it should generate a view X. Hence, I do the 
following inside an sql-script after startup:


DROP TABLE X IF EXISTS ...  
DROP VIEW X IF EXISTS ...   
CREATE VIEW X ...

It works on the first run, but not on a subsequent one, because the view 
X already exists, hence DROP TABLE X fails.


If I switch the first two lines, it fails already during the first run, 
because a table X exists...


DROP VIEW X IF EXISTS ...   
DROP TABLE X IF EXISTS ...  
CREATE VIEW X ...

It is only solvable with two different calls to the database, and error 
handling on the application side.


Intuitively, I (and also others, that I asked) think that this command 
should only fail, if a search for  in combination 
succeeds and the DROP itself fails.


In general my use-case is, that I want to delete an X in a certain 
namespace, where the type is not known in advance. I could query the 
catalog to get that information and then build a procedure to "execute" 
the right DROP command (as Pavel Stehule suggested), but that adds 
complexity to the application code, where it shouldn't be necessary IMHO.


Best regards,
Peter

Re: Capitalization of the name OpenSSL

[Magnus Hagander]

On Thu, Jun 28, 2018 at 4:54 AM, Michael Paquier 
wrote:

> On Thu, Jun 28, 2018 at 12:16:52AM +0200, Daniel Gustafsson wrote:
> > Skimming over SSL code and docs I noticed that we almost always properly
> > capitalize “OpenSSL" when referring to the name of the library, using
> "openssl”
> > for when referring to the cli application.  The attached patch fixes the
> few
> > occurrences where the name is referred to, but which aren’t spelled
> “OpenSSL”.
> > Also moves the link to openssl.org to using https:// as it redirects
> anyways.
>
> I see more that 860 references to openssl as lower case with lots of
> noise around file names and translations.  And I can see that you missed
> two of them:
>
> contrib/pgcrypto/openssl.c: * Check if strong crypto is supported. Some
> openssl installations
>
> src/interfaces/libpq/fe-secure-openssl.c:/* This should exactly match
> openssl's SSL_set_fd except for using my BIO */
>
> I don't think that the comments are worth bothering for anything else
> than HEAD, now should the doc changes be back-patched?  I would tend to
> do so.
>

Normally I'm the biggest promoter for back-patching documentation changes
:) But in the case when it's really just about the capitalization, I think
it's fine to just bother with HEAD for it.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Copy function for logical replication slots

[Michael Paquier]

On Thu, Jun 28, 2018 at 03:34:00PM +0900, Masahiko Sawada wrote:
> On Thu, Jun 28, 2018 at 12:29 PM, Michael Paquier  wrote:
>> Hm.  Shouldn't the original slot copied be owned by the process doing
>> the copy with ReplicationSlotAcquire?
>
> Right, it should do and release it before creating new one.

Yes, or MyReplicationSlot would not like that.

>> There could be some cases where
>> copying a physical slot also makes sense.
> 
> I've thought that but I didn't find concrete use case. That's why I
> started with only logical slot.

Let's imagine the case of a single base backup which is associated to a
given replication slot, and that this backup is then used to spawn
multiple standbys where each one of them needs a separate slot to
consume changes at their pace.  If you can copy the slot used in the
first backup, then both nodes could consume it.  That looks useful to
me to make sure that both slots are based a consistent point.
--
Michael


signature.asc
Description: PGP signature

SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/28/18 09:35, Magnus Hagander wrote:
> No, we absolutely still have SCRAM channel binding.
> 
> *libpq* has no way to *enforce* it, meaning it always acts like our
> default SSL config which is "use it if available but if it's not then
> silently accept the downgrade". From a security perspective, it's just
> as bad as our default ssl config, but unlike ssl you can't configure a
> requirement in 11.

Isn't this similar to what happened whenever we added a new or better
password method?  A MITM that didn't want to bother cracking MD5 could
just alter the stream and request "password" authentication.  Same with
MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
the SCRAM hashing method.  Clearly, we need a more comprehensive
solution for this.

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

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/28/18 09:33, Magnus Hagander wrote:
> Should there be one or more parameters? How should they interact? At
> which level should they be controlled? Limited to SCRAM or other channel
> bindings? Are the different levels of SCRAM to be considered different
> protocols or the same protocol with a tweak? etc.

OK, I'm fine with postponing this.

But before we drop the SCRAM business completely off the open items, I
think we need to consider how TLS 1.3 affects this.

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

Re: Copy function for logical replication slots

[Masahiko Sawada]

On Thu, Jun 28, 2018 at 5:37 PM, Peter Eisentraut
 wrote:
> On 6/28/18 08:47, Michael Paquier wrote:
 There could be some cases where
 copying a physical slot also makes sense.
>>> I've thought that but I didn't find concrete use case. That's why I
>>> started with only logical slot.
>> Let's imagine the case of a single base backup which is associated to a
>> given replication slot, and that this backup is then used to spawn
>> multiple standbys where each one of them needs a separate slot to
>> consume changes at their pace.  If you can copy the slot used in the
>> first backup, then both nodes could consume it.  That looks useful to
>> me to make sure that both slots are based a consistent point.
>

Thank you, that sounds useful. I'll update the patch to include physical slots.

> I think this use case of cloning replicas would also be interesting in
> the logical slot case.
>

+1

Regards,

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

Re: Capitalization of the name OpenSSL

[Daniel Gustafsson]

> On 28 Jun 2018, at 04:54, Michael Paquier  wrote:

> I see more that 860 references to openssl as lower case with lots of
> noise around file names and translations.  And I can see that you missed
> two of them: 

Nice catch, fixed in the attached.

> I don't think that the comments are worth bothering for anything else
> than HEAD, now should the doc changes be back-patched?  I would tend to
> do so.

Not sure if it’s worth any back-patching at all to be honest, but I’ll leave
that call to you.

cheers ./daniel



capitalize_OpenSSL-v2.patch
Description: Binary data

Re: Copy function for logical replication slots

[Masahiko Sawada]

On Thu, Jun 28, 2018 at 12:29 PM, Michael Paquier  wrote:
> On Thu, Jun 28, 2018 at 11:51:20AM +0900, Masahiko Sawada wrote:
>> A use case I imagined is for investigations for example. I mean that
>> when replication collision occurs on subscriber there is no way to see
>> what replicated data is conflicting (perhaps error log helps it but is
>> not detailed) and there is no way to advance a replication origin in
>> order to exactly skip to apply conflicting data. By creating a new
>> logical slot with a different output plugin at the same LSN, we can
>> see what data a replication slot will decode (and send) and those LSNs
>> as well. This function will help for that purpose.
>
> Hm.  Shouldn't the original slot copied be owned by the process doing
> the copy with ReplicationSlotAcquire?

Right, it should do and release it before creating new one.

> There could be some cases where
> copying a physical slot also makes sense.

I've thought that but I didn't find concrete use case. That's why I
started with only logical slot.

Attached v2 patch.

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


v2-0001-Copy-logical-replication-slot.patch
Description: Binary data

Re: ALTER TABLE on system catalogs

[Peter Eisentraut]

On 6/28/18 01:10, Michael Paquier wrote:
> On Wed, Jun 27, 2018 at 01:37:33PM -0700, Andres Freund wrote:
>> On 2018-06-27 22:31:30 +0200, Peter Eisentraut wrote:
>>> I propose that we instead silently ignore attempts to add TOAST tables
>>> to shared and system catalogs after bootstrapping.
>>
>> That seems like an extremely bad idea to me.  I'd rather go ahead and
>> just add the necessary toast tables than silently violate preconditions
>> that code assumes are fulfilled.
> 
> Agreed.  Joe Conway was working on a patch to do exactly that.  I was
> personally looking for the possibility of having one with pg_authid in
> v12 :)

OK, that would change things a bit, in that the silent addition of a
TOAST table would no longer be a problem, but it wouldn't fix the other
scenarios that end up in an error.  If such a patch is forthcoming, we
can revisit this again afterwards.

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

Re: Changing the autovacuum launcher scheduling; oldest table first algorithm

[Masahiko Sawada]

On Wed, Mar 14, 2018 at 11:29 AM, Masahiko Sawada  wrote:
> On Tue, Mar 6, 2018 at 11:27 PM, Alvaro Herrera  
> wrote:
>> Hello
>>
>> I haven't read your respective patches yet, but both these threads
>> brought to memory a patch I proposed a few years ago that I never
>> completed:
>>
>> https://www.postgresql.org/message-id/flat/20130124215715.GE4528%40alvh.no-ip.org
>
> Thank you for sharing the thread.
>
>>
>> In that thread I posted a patch to implement a prioritisation scheme for
>> autovacuum, based on an equation which was still under discussion when
>> I abandoned it.  Chris Browne proposed a crazy equation to mix in both
>> XID age and fraction of dead tuples; probably that idea is worth
>> studying further.  I tried to implement that in my patch but I probably
>> didn't do it correctly (because, as I recall, it failed to work as
>> expected).  Nowadays I think we would also consider the multixact freeze
>> age, too.
>>
>> Maybe that's worth giving a quick look in case some of the ideas there
>> are useful for the patches now being proposed.
>
> Yeah, that's definitely useful for the patches. I'll look at this and
> will discuss that here.
>

I read  that discussion. If I understand correctly, the patch proposed
in that discussion mainly focuses on table-selection algorithm for
each AV worker. But the patch I proposed also changes it but the main
part of this patch is to change the database-selection algorithm by AV
launcher (sorry, maybe the subject leads misleading). The problem I'd
like to deal with is, when there is at least one database needing
anti-wraparound vacuum it concentrates launching new AV workers on one
database.

If we change the table-selection algorithm to the "oldest table first"
algorithm in that case, we can deal with that problem by this patch.
However, I think it's worthwhile to consider another table-selection
algorithm such as the proposal on that old thread. So I think it would
be better to think these issue separately. That is, we can change the
database-selection algorithm in order to prevent the concentration
problem while not changing table-selection algorithm.

This brought me another idea; having AV workers report its autovacuum
results at its AV worker slot so that they can teach autovacuum status
to AV launcher. For example, each AV worker can report the following
information.

* Database oid
* The number of tables needing vacuum.
* The number of vacuumed tables.
* The number of skipped tables due to being processed by other AV workers.
* Timestamp of last update

If there is an up-to-date information meaning either that there is no
tables needing vacuum or that there is only table needing vacuum but
being vacuumed by other worker, AV launcher can launches new one to
other database.

Regards,

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

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Wed, Jun 27, 2018 at 7:24 PM, Alvaro Herrera 
wrote:

> Going over this thread a little bit I'm confused about what is being
> proposed.  I think I understand that we no longer think we have have
> SCRAM channel binding.  I hope that doesn't mean we don't have SCRAM
> itself.  However, in terms of the Postgres release proper, what do we
> need to do?  There is still an open item about this, and I had the
> impression that if we simply demoted channel binding from a pg11 major
> feature to barely a footnote that somebody can implement it with some
> hypothetical future JDBC driver that supports the option, then we're
> done.
>
> Am I mistaken?
>

No, we absolutely still have SCRAM channel binding.

*libpq* has no way to *enforce* it, meaning it always acts like our default
SSL config which is "use it if available but if it's not then silently
accept the downgrade". From a security perspective, it's just as bad as our
default ssl config, but unlike ssl you can't configure a requirement in 11.

There is nothing preventing a third party driver like jdbc or npgsql to
implement a way to enforce it. I would generally recommend they wait for
the outcome of the discussion about parameters and names in order to
implement the same semantics, but they don't have to wait for the next
postgres release.

It doesn't affect the having of SCRAM at all. That one is still there, and
has been since 10.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/28/18 09:35, Magnus Hagander wrote:
> No, we absolutely still have SCRAM channel binding.
> 
> *libpq* has no way to *enforce* it, meaning it always acts like our
> default SSL config which is "use it if available but if it's not then
> silently accept the downgrade". From a security perspective, it's just
> as bad as our default ssl config, but unlike ssl you can't configure a
> requirement in 11.

Isn't this similar to what happened whenever we added a new or better
password method?  A MITM that didn't want to bother cracking MD5 could
just alter the stream and request "password" authentication.  Same with
MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
the SCRAM hashing method.  Clearly, we need a more comprehensive
solution for this.

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

Re: Generating partitioning tuple conversion maps faster

[Alexander Kuzmenkov]

On 06/25/2018 08:00 AM, David Rowley wrote:

I'd have expected Test 6 to do about 480-500tps. Adding debug to check
why it's not revealed that the search is doing what's expected. I'm
unsure why it has not improved more.


I think I found one possible reason for this slowness. Your patch 
behaves as expected when there is a dropped output column, but does one 
extra comparison when there is a dropped input column. This backwards 
conversion is called from ExecInitRoutingInfo. To fix this, I'd just 
keep a persistent inner loop counter (see the attached patch).


Still, fixing this doesn't improve the performance. According to perf 
report, updatepd.sql spends 25% of time in strcmp, and updatep.sql about 
0.5%, but they are comparing the same column names, even with the same 
alignment and relative offsets. I'm completely puzzled by this.


As a side thought, we wouldn't have to go through this if we had a hash 
table that is easy to use, or perhaps string interning in catcache.


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

diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index 2d0d2f4..573ddd82 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -297,6 +297,7 @@ convert_tuples_by_name_map(TupleDesc indesc,
 	AttrNumber *attrMap;
 	int			n;
 	int			i;
+	int nextInput = -1;
 
 	n = outdesc->natts;
 	attrMap = (AttrNumber *) palloc0(n * sizeof(AttrNumber));
@@ -315,7 +316,13 @@ convert_tuples_by_name_map(TupleDesc indesc,
 		atttypmod = outatt->atttypmod;
 		for (j = 0; j < indesc->natts; j++)
 		{
-			Form_pg_attribute inatt = TupleDescAttr(indesc, j);
+			Form_pg_attribute inatt;
+
+			nextInput++;
+			if (nextInput >= indesc->natts)
+nextInput = 0;
+
+			inatt = TupleDescAttr(indesc, nextInput);
 
 			if (inatt->attisdropped)
 continue;
@@ -330,7 +337,7 @@ convert_tuples_by_name_map(TupleDesc indesc,
 	   attname,
 	   format_type_be(outdesc->tdtypeid),
 	   format_type_be(indesc->tdtypeid;
-attrMap[i] = (AttrNumber) (j + 1);
+attrMap[i] = (AttrNumber) (nextInput + 1);
 break;
 			}
 		}

Re: Tips on committing

[Robert Haas]

On Thu, Jun 28, 2018 at 11:20 AM, Andres Freund  wrote:
>> Apparently, there's a recent trend to credit patch authors using
>> "Co-authored-by".  Should we use that too?
>> https://stackoverflow.com/a/41847267/
>
> I just put multiple people into Authors, with order roughly implying the
> amount of work. Don't really see a reason to split it off further?

One of the reasons that I've stuck with free-form text for denoting
authors rather than using tags is precisely so I can try to clarify
relative levels of contribution.   I think being able to have multiple
Author: tags -- each such author being a major author -- and also
multiple Co-authored-by tags -- each such coauthor having made a
relatively small contribution -- would be an adequate amount of
distinction for almost all cases that I deal with.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Tips on committing

[Alvaro Herrera]

I'm not sure pgsql-committers was the right audience. Cross-posting to
pg-hackers.

On 2018-Jun-28, Bruce Momjian wrote:

>  2  Reported-by:
>  5  Author:
>  6  Reviewed-by:
>  7  Tested-by:

Should these include email addresses?

I've also used "Diagnosed-by" to credit a person who spent time studying
a bug's mechanism and how to fix it.  Sometimes that's the same as
Reported-by, but I think the weight is quite different.

Apparently, there's a recent trend to credit patch authors using
"Co-authored-by".  Should we use that too?
https://stackoverflow.com/a/41847267/

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

Re: Tips on committing

[Andres Freund]

On 2018-06-28 11:14:38 -0400, Alvaro Herrera wrote:
> I'm not sure pgsql-committers was the right audience. Cross-posting to
> pg-hackers.
> 
> On 2018-Jun-28, Bruce Momjian wrote:
> 
> >  2  Reported-by:
> >  5  Author:
> >  6  Reviewed-by:
> >  7  Tested-by:
> 
> Should these include email addresses?

I'm not sure it's that helpful, they tend to be out of date pretty soon.


> I've also used "Diagnosed-by" to credit a person who spent time studying
> a bug's mechanism and how to fix it.  Sometimes that's the same as
> Reported-by, but I think the weight is quite different.

Yea, same here.


> Apparently, there's a recent trend to credit patch authors using
> "Co-authored-by".  Should we use that too?
> https://stackoverflow.com/a/41847267/

I just put multiple people into Authors, with order roughly implying the
amount of work. Don't really see a reason to split it off further?

Greetings,

Andres Freund