Re: [HACKERS] Does pg_upgrade really support "make installcheck"?

[Neha Khatri]

On 6/7/17, Tom Lane  wrote:
> src/bin/pg_upgrade/TESTING claims (much further down in the file
> than I'd like):
>
>   The shell script test.sh in this directory performs more or less this
>   procedure.  You can invoke it by running
>   make check
>   or by running
>   make installcheck
>   if "make install" (or "make install-world") were done beforehand.
>
> However, the second alternative doesn't really work:
>
> $ make installcheck
> make: Nothing to be done for `installcheck'.
> $
>
> Did this ever work, or could it easily be made to work?

It seems it would work if the following two lines are uncommented from
the src/bin/pg_upgrade/Makefile:

  # disabled because it upsets the build farm
  # installcheck: test.sh
  #  MAKE=$(MAKE) bindir=$(bindir) libdir=$(libdir) $(SHELL) $<

As the comment says, it was purposely disabled, probably to avoid
failure on cetain build farm members.
Attached the result of make installcheck after enabling the
intallcheck target (just to be sure if that is what you are looking
for).

> If not, we need to fix that documentation.

If the attached is result is what you are after, should the
documentation be updated to mention that make installcheck is
purposely disabled, providing the reason for it. Or, should the
intallcheck target be enabled in the Makefile to find out if specific
buildfarm members still complain about it.

-- 
Regards,
Neha


pgu_installcheck.log
Description: Binary data

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

Re: [HACKERS] PG 10 release notes

[Neha Khatri]

On Mon, May 15, 2017 at 12:45 PM, Bruce Momjian  wrote:

> On Thu, May 11, 2017 at 11:50:03PM -0400, Tom Lane wrote:
> > Michael Paquier  writes:
> > > Bruce, the release notes do not mention yet that support for cleartext
> > > passwords is removed. This has been done recently by Heikki in
> > > eb61136d. This has as consequence that CREATE ROLE PASSWORD
> > > UNENCRYPTED returns an error, and that password_encryption loses its
> > > value 'off' compared to last releases.
> >
> > The release notes only say that they're current through 4-22.  The
> > usual process is to update them in batches every so often.  It'd be
> > great if Bruce has time to do another round before Monday, but the
> > current situation is not really broken.
>
> Done.  Applied patch attached.
>
>
The patch introduced one release note item twice in
https://www.postgresql.org/docs/10/static/release-10.html :


   -

   Rename WAL-related functions and views to use lsn instead of location (David
   Rowley)
   -

   RenameWAL-related functions and views to use lsn instead of location (David
   Rowley)

Perhaps just one entry is good.

Regards,
Neha

[HACKERS] Typo in xlogfuncs.c [WAS Re: Incorrect mention of pg_xlog_switch() in xlogfuncs.c]

[Neha Khatri]

Simplifying  $subject. There are typos in xlogfuncs.c. So Either

s/pg_xlog_switch/pg_switch_wal

Or

Remove "pg_xlog_switch" from the comments.

Attached patches both ways.

Regards,
Neha

On Sat, May 20, 2017 at 1:08 AM, Neha Khatri <nehakhat...@gmail.com> wrote:

> While reading some code, noticed that the headers of functions
> pg_walfile_name_offset() and pg_walfile_name() incorrecty refer
> pg_xlog_switch() since the inception of code in commit 704ddaaa.
>
> In PG10 implementation, actual name of the referred function is
> pg_switch_wal(). So either refer the correct name in the function
> header or remove the other function referral from the function header.
>
>


remove_incorrect_function_referral.patch
Description: Binary data


correctly_refer_pg_switch_wal.patch
Description: Binary data

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

Re: [HACKERS] wal_level > WAL_LEVEL_LOGICAL

[Neha Khatri]

On Wed, 24 May 2017 at 10:29 pm, Robert Haas <robertmh...@gmail.com> wrote:

> On Mon, May 22, 2017 at 9:08 AM, Neha Khatri <nehakhat...@gmail.com>
> wrote:
> > As per my understabding, current postgres server supports only three
> > values for wal_level i.e. 'minimal' , 'replica' or 'logical'. But
> > following error message brought to notice that there are various code
> > spots that try to look for wal_level >= WAL_LEVEL_LOGICAL:
>
> I suspect that this was intended as future-proofing.  I think it's
> actually very reasonable to write the internal tests that way,


Agreed. Share the same thought and also started another thread just for the
user visible error message improvement [1]. In that thread the error
message is perceived to be correct.

but it
> does seem strange that it's crept into the user-visible error
> messages.


Yep, this seems useful for developer but not the end user.

[1]
https://www.postgresql.org/message-id/CAFO0U%2B_y8AyAcQLiF3S1i6yCNuYrcLNEd-BbzCuHiGOSejW%3D2A%40mail.gmail.com

Regards,
Neha
-- 
Cheers,
Neha

Re: [HACKERS] Improve logical decoding error message (was wal_level > WAL_LEVEL_LOGICAL)

[Neha Khatri]

On Tue, 23 May 2017 at 10:55 am, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:

> Neha Khatri wrote:
> > On Tue, May 23, 2017 at 10:26 AM, Michael Paquier <
> michael.paqu...@gmail.com
>
> > > There is no wal_level higher than logical, so the current sense looks
> > > perfectly fine to me.
> >
> > If there is no wal_level higher than logical, should the following error
> > message indicate to set it >= logical.
> >
> >  select * from
> > pg_create_logical_replication_slot('regression_slot','test_decoding');
> >  ERROR:  logical decoding requires wal_level >= logical
>
> I think it's purposefully ambiguous to cover a possible future
> extension.
>

Should documentation also have similar statement and indicate future
possibility.

What is the benefit of having it just in error message.

Regards,
Neha
-- 
Cheers,
Neha

Re: [HACKERS] Improve logical decoding error message (was wal_level > WAL_LEVEL_LOGICAL)

[Neha Khatri]

On Tue, May 23, 2017 at 10:26 AM, Michael Paquier <michael.paqu...@gmail.com
> wrote:

> On Tue, May 23, 2017 at 8:08 AM, Neha Khatri <nehakhat...@gmail.com>
> wrote:
> > The Logical Decoding example in the documentation says:
> >
> >   "Before you can use logical decoding, you must set wal_level to logical
> > and max_replication_slots to at least 1."
> >
> > But above error message is not exactly consistent with this
> documentation.
> > Would it make sense to keep the error message and the documentation
> > consistent like the attached.
>
> There is no wal_level higher than logical, so the current sense looks
> perfectly fine to me.
>

If there is no wal_level higher than logical, should the following error
message indicate to set it >= logical.

 select * from
pg_create_logical_replication_slot('regression_slot','test_decoding');
 ERROR:  logical decoding requires wal_level >= logical

Regards,
Neha

[HACKERS] Improve logical decoding error message (was wal_level > WAL_LEVEL_LOGICAL)

[Neha Khatri]

On Mon, May 22, 2017 at 11:08 PM, Neha Khatri <nehakhat...@gmail.com> wrote:

> As per my understabding, current postgres server supports only three
> values for wal_level i.e. 'minimal' , 'replica' or 'logical'. But
> following error message brought to notice that there are various code
> spots that try to look for wal_level >= WAL_LEVEL_LOGICAL:
>
>   select * from pg_create_logical_replication_slot('regression_slot',
> 'test_decoding');
>   ERROR:  logical decoding requires wal_level >= logical
>

The Logical Decoding example in the documentation says:

  "Before you can use logical decoding, you must set wal_level
<https://www.postgresql.org/docs/10/static/runtime-config-wal.html#guc-wal-level>
 to logical and max_replication_slots
<https://www.postgresql.org/docs/10/static/runtime-config-replication.html#guc-max-replication-slots>
to
at least 1."

But above error message is not exactly consistent with this documentation.
Would it make sense to keep the error message and the documentation
consistent like the attached.

Regards,
Neha


improve_err_message.patch
Description: Binary data

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

[HACKERS] wal_level > WAL_LEVEL_LOGICAL

[Neha Khatri]

As per my understabding, current postgres server supports only three
values for wal_level i.e. 'minimal' , 'replica' or 'logical'. But
following error message brought to notice that there are various code
spots that try to look for wal_level >= WAL_LEVEL_LOGICAL:

  select * from pg_create_logical_replication_slot('regression_slot',
'test_decoding');
  ERROR:  logical decoding requires wal_level >= logical

The code locations that look for/expect wal_level > WAL_LEVEL_LOGICAL are:

 heapam.c 7690 * This is only used in wal_level >=
WAL_LEVEL_LOGICAL, and only for catalog
 logical.c   83 errmsg("logical decoding requires wal_level >=
logical")));
 standby.cLogStandbySnapshot   950 if (wal_level
>= WAL_LEVEL_LOGICAL)
 xlog.h   XLogLogicalInfoActive162 #define
XLogLogicalInfoActive() (wal_level >= WAL_LEVEL_LOGICAL)

Since postgres does not allow wal_level > WAL_LEVEL_LOGICAL, the above
code locations should be modified like:

 s/>=/=

Thoughts/Suggestions?

Regards,
Neha


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

[HACKERS] Incorrect mention of pg_xlog_switch() in xlogfuncs.c

[Neha Khatri]

While reading some code, noticed that the headers of functions
pg_walfile_name_offset() and pg_walfile_name() incorrecty refer
pg_xlog_switch() since the inception of code in commit 704ddaaa.

In PG10 implementation, actual name of the referred function is
pg_switch_wal(). So either refer the correct name in the function
header or remove the other function referral from the function header.

Attaching patches for both the ways.
-- 
Regards,
Neha
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index b3223d6..463f668 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -448,8 +448,7 @@ pg_last_wal_replay_lsn(PG_FUNCTION_ARGS)
 }
 
 /*
- * Compute an xlog file name and decimal byte offset given a WAL location,
- * such as is returned by pg_stop_backup() or pg_xlog_switch().
+ * Compute an xlog file name and decimal byte offset given a WAL location.
  *
  * Note that a location exactly at a segment boundary is taken to be in
  * the previous segment.  This is usually the right thing, since the
@@ -514,8 +513,7 @@ pg_walfile_name_offset(PG_FUNCTION_ARGS)
 }
 
 /*
- * Compute an xlog file name given a WAL location,
- * such as is returned by pg_stop_backup() or pg_xlog_switch().
+ * Compute an xlog file name given a WAL location.
  */
 Datum
 pg_walfile_name(PG_FUNCTION_ARGS)
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index b3223d6..fb905c0 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -449,7 +449,7 @@ pg_last_wal_replay_lsn(PG_FUNCTION_ARGS)
 
 /*
  * Compute an xlog file name and decimal byte offset given a WAL location,
- * such as is returned by pg_stop_backup() or pg_xlog_switch().
+ * such as is returned by pg_stop_backup() or pg_switch_wal().
  *
  * Note that a location exactly at a segment boundary is taken to be in
  * the previous segment.  This is usually the right thing, since the
@@ -515,7 +515,7 @@ pg_walfile_name_offset(PG_FUNCTION_ARGS)
 
 /*
  * Compute an xlog file name given a WAL location,
- * such as is returned by pg_stop_backup() or pg_xlog_switch().
+ * such as is returned by pg_stop_backup() or pg_switch_wal().
  */
 Datum
 pg_walfile_name(PG_FUNCTION_ARGS)

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

Re: [HACKERS] If subscription to foreign table valid ?

[Neha Khatri]

On Fri, May 12, 2017 at 11:58 PM, Petr Jelinek  wrote:

> On 11/05/17 15:43, Petr Jelinek wrote:
> > Hi,
>
> >
> > We do check for this, but only during replication which we have to do
> > because the fact that relation 't' was foreign table during ALTER
> > SUBSCRIPTION does not mean that it won't be something else half hour
> later.
> >
> > I think it does make sense to add check for this into CREATE/ALTER
> > SUBSCRIBER though so that user is informed immediately about the mistake
> > rather than by errors in the logs later.
> >
> > I'll look into writing patch for this. I don't think it's beta blocker
> > though.
> >
>
> So I moved the relkind check to single function and call it from all the
> necessary places. See the attached
>
>
With this patch the error will be like this:

  logical replication target relation public.t is not a table

But it is possible that the referred table is Foreign Table of Partitioned
table (so actually the referred object is indeed a table).
Would it make sense to specify in the message that the table is not a
normal table or something in that line?

Regards,
Neha

Re: [HACKERS] If subscription to foreign table valid ?

[Neha Khatri]

>
>
>
> I sent my version of patch in parallel. I think we don't need to do the
> relation open like you did, all the info is in syscache.
>

That's right.

>
Regards,
Neha

Re: [HACKERS] Time based lag tracking for logical replication

[Neha Khatri]

On Sat, May 13, 2017 at 12:04 AM, Petr Jelinek  wrote:

> > After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get
> > following error while executing CREATE SUBSCRIPTION:
> >
> > CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost
> > user=neha port=5432' PUBLICATION mypub;
> > NOTICE:  synchronized table states
> > ERROR:  could not create replication slot "sub1": ERROR:  could not
> > load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so":
> > /home/neha/postgres/PGCurrentInstall/lib/pgoutput.so: undefined
> > symbol: OutputPluginUpdateProgress
> >
>
> Hmm, that sounds like partial rebuild/install (old postgres binary with
> new pgoutput one).
>

That's right.  Thanks.

Neha

Re: [HACKERS] If subscription to foreign table valid ?

[Neha Khatri]

[Correction below]

On 5/12/17, Neha Khatri <nehakhat...@gmail.com> wrote:
> On 5/11/17, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote
>
>> However, the foreign tables indeed can't be subscribed.

Yes, I suspect that a user would _not_ want to subcribe a foreign
table in real world.


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

Re: [HACKERS] If subscription to foreign table valid ?

[Neha Khatri]

On 5/11/17, Petr Jelinek  wrote:
> Hi,
>
> On 11/05/17 14:25, tushar wrote:
>> Hi,
>>
>> I observed that -we cannot publish "foreign table" in Publication
>>
>> but same thing is not true for Subscription
>>
>> postgres=# create foreign table t (n int) server db1_server options
>> (table_name 't');
>> CREATE FOREIGN TABLE
>> postgres=# alter subscription sub refresh publication ;
>> NOTICE:  added subscription for table public.t
>> ALTER SUBSCRIPTION
>>
>> Is this an expected behavior ?   if we cannot publish then how  can we
>> add subscription for it.

The above foreign table subscription succeeds only if the publisher
has published a same named normal table i.e.
  create table t (n int);
  CREATE PUBLICATION mypub FOR TABLE t;

I think in current implemetation of LogicalRep. it is users
responsibility to match the table definition at publisher and
subscriber. Subscriber can not determine by itself what publisher has
defined. This usecase does not align with this assumption.


> However, the foreign tables indeed can't be subscribed.

I suspect that a user would want to subcribe a foreign table in real world.

> I think it does make sense to add check for this into CREATE/ALTER
> SUBSCRIBER though so that user is informed immediately about the mistake
> rather than by errors in the logs later.

Yes, system should prohibit such operation though.
I tried to write to a patch to achieve this. It disalows subscription
of relation other than RELKIND_RELATION.
Attached is the patch. Comments?

Regards,
Neha
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index b76cdc5..f6013da 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,9 +440,20 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 			{
 RangeVar   *rv = (RangeVar *) lfirst(lc);
 Oid			relid;
+Relation		relation;
 
 relid = RangeVarGetRelid(rv, AccessShareLock, false);
 
+relation = RelationIdGetRelation(relid);
+
+if (RelationGetForm(relation)->relkind != RELKIND_RELATION)
+	ereport(ERROR,
+			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+			 errmsg("\"%s\" is not a normal table",
+	RelationGetRelationName(relation)),
+			 errdetail("Only normal tables can be subscribed.")));
+RelationClose(relation);
+
 SetSubscriptionRelState(subid, relid, table_state,
 		InvalidXLogRecPtr);
 			}
@@ -553,8 +564,21 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	{
 		RangeVar   *rv = (RangeVar *) lfirst(lc);
 		Oid			relid;
+		Relation		relation;
 
 		relid = RangeVarGetRelid(rv, AccessShareLock, false);
+
+		relation = RelationIdGetRelation(relid);
+
+		if (RelationGetForm(relation)->relkind != RELKIND_RELATION)
+			ereport(NOTICE,
+	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+	 errmsg("\"%s\" is not a normal table",
+		RelationGetRelationName(relation)),
+	 errdetail("Only normal tables can be subscribed.")));
+
+		RelationClose(relation);
+
 		pubrel_local_oids[off++] = relid;
 
 		if (!bsearch(, subrel_local_oids,

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

Re: [HACKERS] Time based lag tracking for logical replication

[Neha Khatri]

On Fri, May 12, 2017 at 8:19 PM, Simon Riggs  wrote:
>
> On 11 May 2017 at 18:29, Simon Riggs  wrote:
> > On 11 May 2017 at 18:13, Andres Freund  wrote:
> >
> >>>New patch, v3.
> >>>
> >>>Applying in 90 minutes, barring objections.
> >>
> >> Could you please wait till tomorrow?  I've bigger pending fixes for 
> >> related code pending/being tested that I plan to push today.  I'd also 
> >> like to take a look before...
> >
> > Sure.
>
> The changes I've added are very minor, so I'm not expecting debate.
> The main part of the patch is the same as Petr posted 19days ago.
>
> I'm travelling now, so after waiting till tomorrow as you requested I
> have committed the patch.
>

Prior to this commit CREATE SUBSCRIPTION used to work smoothly.

After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get
following error while executing CREATE SUBSCRIPTION:

CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost
user=neha port=5432' PUBLICATION mypub;
NOTICE:  synchronized table states
ERROR:  could not create replication slot "sub1": ERROR:  could not
load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so":
/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so: undefined
symbol: OutputPluginUpdateProgress

Regards
Neha


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

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[Neha Khatri]

On Fri, May 12, 2017 at 12:56 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Neha Khatri <nehakhat...@gmail.com> writes:
> > [In case forgotten] pg_controdata and pg_waldump interfaces should also
> be
> > considered for this standardization.
>
> > Following are pg_controldata interfaces that might require change:
>
> >   Latest checkpoint location:
> >   Prior checkpoint location:
> >   Latest checkpoint's REDO location:
> >   Minimum recovery ending location:
> >   Backup start location:
> >   Backup end location:
>
> My inclination is to leave these messages alone.  They're not really
> inconsistent with anything.  Where we seem to be ending up is that
> "lsn" will be used in things like function and column names, but
> documentation will continue to spell out phrases like "WAL location".
>
> There is another open thread about converting said phrases to be
> more consistent --- a lot of them currently say "transaction log
> location", which is not a very good choice because it invites
> confusion with pg_xact nee pg_clog.  But I think that's mostly
> just documentation changes, and in any case it's better done as
> a separate patch.
>
>
Are you indicating that the above phrases do not require change because
those are consistent with other references. Or the other thread [1]
(renaming 'transaction log') should take care of it.

Regards,
Neha

[1]
https://www.postgresql.org/message-id/89ba433e-8990-0aad-238f-55e1d7280ece%402ndquadrant.com

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[Neha Khatri]

On Sat, May 6, 2017 at 4:09 PM, Tom Lane  wrote:

> Noah Misch  writes:
> > On Fri, May 05, 2017 at 03:36:39AM +1200, David Rowley wrote:
> >> Do any of the committers who voted for this change feel inclined to
> >> pick this patch up?
>
> > I'll echo that question.  This open item lacks a clear owner.  One might
> argue
> > that 806091c caused it by doing the backward-compatibility breaks that
> > inspired this patch, but that's a link too tenuous to create ownership.
>
> If no one else takes this, I will pick it up --- but I don't anticipate
> having any time for it until after Monday's back-branch release wraps.
>

[In case forgotten] pg_controdata and pg_waldump interfaces should also be
considered for this standardization.

Following are pg_controldata interfaces that might require change:

  Latest checkpoint location:
  Prior checkpoint location:
  Latest checkpoint's REDO location:
  Minimum recovery ending location:
  Backup start location:
  Backup end location:

The pg_waldump help options(and probably documentation) would also require
change:
  -e, --end=RECPTR   stop reading at log position RECPTR
  -s, --start=RECPTR start reading at log position RECPTR

Regards,
Neha

Re: [HACKERS] Duplicate usage of tablespace location?

[Neha Khatri]

As Kyotaro san pointed out, the commit 22817041 started allowing creation
of multiple "tablespace version directories" in same location. However the
original purpose of that commit was to allow that just for the upgrade
purpose. So couple of points:
- The commit violated the requirement of emptiness of the tablespace
location directory.
(Though it is still prevented to create multiple tablespaces belonging
to one server, in same location.)
- The comment did not document this change in specification.

Probably it was not anticipated at that time that a user could create the
tablespaces for different server version at the same location.

Now that this behaviour is present in field for a while, there is
likelihood of having systems with tablespaces for two different versions,
in same location. To avoid the problem reported in [1] for such systems,
here are couple of alternative approaches:

1. Allow creation of multiple tablespaces in single location for different
server versions, but not the same version(exception).
a) Also allow this capability in utilities like pg_basebackup( and others
that update tablespaces) .
b) Update the documentation about this specification change.

I don't see this breaking any backwards compatibility.

2. Retain the current base rule of creating Tablespaces i.e. "The location
must be an existing, empty directory". This means:
a) For the future release, have a strict directory emptiness check while
creating new tablespace.
b) Only during upgrade, allow creation of multiple tablepaces in same
location .
c) Document the fact that only during upgrade the system would create
multiple tablespaces in same location.
d) Provide a flexibility to change the location of an existing tablespace,
something like:
ALTER TABLESPACE tblspcname SET LOCATION '/path/to/newlocation'
[where newlocation is an existing empty direcotry]

With the altered location of a tablespace it should be possible to perform
the pg_basebackup successfully.
I noticed some solutions for moving PostgreSQL tablesspaces, on internet.
But some are slow, others cause incompatibility for tools like pgAdmin. I
am not able to find any discussion about moving tablespace location in
mailing lists too. So I am not sure if there is already any conclusion
about supporting or not supporting ALTER TABLESPACE LOCATION.
To me, the first approach above looks like providing more independence to
the user about choice of tablespace location. Also, it is not clear that
why the directory emptiness rule was introduced in first place. Any insight
on that will be useful.

Regards,
Neha

[1]https://www.postgresql.org/message-id/2008148.rxBNyNRHPZ@peanuts2

Cheers,
Neha

On Fri, Apr 7, 2017 at 11:02 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> I don't mean that this is the only or best way to go.
>
> I apologize for the possible lack of explanation.
>
> At Thu, 06 Apr 2017 12:03:51 -0400, Tom Lane  wrote in
> <21084.1491494...@sss.pgh.pa.us>
> > Kyotaro HORIGUCHI  writes:
> > > I noticed by the following report, PostgreSQL can share the same
> > > directory as tablespaces of two servers with different
> > > pg-versions.
> >
> > > https://www.postgresql.org/message-id/2008148.rxBNyNRHPZ@peanuts2
> >
> > > 8.4 checked that the tablespace location is empty, but from 9.0,
> > > the check is replaced with creating a PG_PGVER_CATVER
> > > subdirectory. This works for multiple servers with the same
> > > version, but don't for servers with different versions.
> >
> > Please explain why you think it doesn't work.  This patch seems to
> > be reverting an intentional behavioral change, and you haven't
>
> I understand that the change is for in-place upgrade, not for
> sharing a tablespace diretory between two version of PostgreSQL
> servers. It actually rejects the second server with the same
> version to come. If this is correct, it doesn't seem right to
> accept the second server of the different version.
>
> If we allow sharing of the directory, theoretically we can allow
> the same between the same version of servers by adding system
> identifier in the subdirectory name.
>
>
> > really explained why we'd want to.  It certainly doesn't look like
> > it addresses the referenced complaint about pg_basebackup behavior.
>
> My point is that "the direcotry for newly created tablespace is
> really reuiqred to be literary empty or not?"
>
> Practically it doesn't need to be empty and succesful creation of
> PG_VER_CATVER directory is enough as the current implement
> does. If we take this way the documentation and pg_basebackup
> should be changed and the problem will be resolved as the result.
>
> https://www.postgresql.org/docs/9.6/static/manage-ag-tablespaces.html
>
> - The location must be an existing, empty directory that is owned
> - by the PostgreSQL operating system user. All objects subsequently
> - created within the tablespace will be stored in files underneath
> - this 

Re: [HACKERS] Description of create_singleton_array()

[Neha Khatri]

On Tue, May 2, 2017 at 5:47 AM, Tom Lane  wrote:

>
>
> Well, now that we've been burnt once by the specific call site moving,
> I think we should learn from experience and not have this say where
> it's called from.  That's a lousy substitute for defining the API
> expectations explicitly, anyway.
>

Agreed.

>
> Your proposed patch tries to improve that, but the result isn't
> necessarily a "1-D array" --- it's a one-element array, with possibly
> a higher number of dimensions than 1.  (Not really sure why we thought
> flexibility in the number of dimensions was useful, but there it is.)
>

Ah, yes. The function in itself has the capability to allow m-D array where
'm' could be more than 1. I kind of missed this fact because the only
caller of the function, attempts to create a 1-D array.

>
> I wonder if we wouldn't be better off to get rid of this function entirely.
> It seems like it's not providing any real increment of simplicity over a
> direct call to construct_md_array, since text_to_array could perfectly
> well hard-wire the array element storage properties, as we do in very many
> other places.  And it's a bug waiting to happen, looks like.


Yesterday, while prying into the definition of this function it did occur
to me that whether there is an additional benefit of this function over
construct_md_array. Yes, it looked like that construct_md_array could be
used in lieu of create_singleton_array.

Regards,
Neha

[HACKERS] Description of create_singleton_array()

[Neha Khatri]

Is it intentional to have the existing $SUBJECT.

The commit 33f43725

updated
the function text_to_array() such that it does not directly invoke
create_singleton_array(). But $SUBJECT was not updated.

If it is not intentional then is it fine to update the description like
attached.

Regards,
Neha


correct-desc-create-singleton-array.patch
Description: Binary data

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

Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

[Neha Khatri]

On Thu, Apr 27, 2017 at 4:01 AM, Tom Lane  wrote:

> Jeff Janes  writes:
> > This gives me compiler warning:
> > launcher.c: In function 'logicalrep_worker_launch':
> > launcher.c:257: warning: 'slot' may be used uninitialized in this
> function
>
> Yeah, me too.  Fix pushed.


Somewhat off the mark, but related to warning above, would you get similar
warning for "address" in ProcessUtilitySlow() in utility.c.

Regards,
Neha

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Neha Khatri]

On Tue, Apr 11, 2017 at 1:16 AM, Robert Haas  wrote:

>
> 1. Forget BGW_NEVER_RESTART workers in
> ResetBackgroundWorkerCrashTimes() rather than leaving them around to
> be cleaned up after the conclusion of the restart, so that they go
> away before rather than after shared memory is reset.


Now with this, would it still be required to forget BGW_NEVER_RESTART
workers in maybe_start_bgworker():

if (rw->rw_crashed_at != 0)
{
if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
{
ForgetBackgroundWorker();
continue;
}
 ..
}

Regards,
Neha

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Neha Khatri]

On Wed, Apr 5, 2017 at 5:34 PM, Kuntal Ghosh <kuntalghosh.2...@gmail.com>
 wrote:

> On Tue, Apr 4, 2017 at 12:16 PM, Neha Khatri <nehakhat...@gmail.com>
> wrote:
>
> > I feel there should be an assert if
> > (BackgroundWorkerData->parallel_register_count -
> > BackgroundWorkerData->parallel_terminate_count) > max_parallel_workers)
> >
> Backend 1 > SET max_parallel_worker = 8;
> Backend 1 > Execute a long running parallel query q1 with number of
> parallel worker spawned is say, 4.
> Backend 2> SET max_parallel_worker = 3;
> Backend 2 > Execute a long running parallel query q2 with number of
> parallel worker spawned > 0.
>
> The above assert statement will bring down the server unnecessarily
> while executing q2.


Right, with multiple backends trying to fiddle with max_parallel_workers,
that might bring the server down with the said assert:
Assert(parallel_register_count - parallel_terminate_count <=
max_parallel_workers)

The problem here seem to be the change in the max_parallel_workers value while
the parallel workers are still under execution. So this poses two questions:

1. From usecase point of view, why could there be a need to tweak the
max_parallel_workers exactly at the time when the parallel workers are at
play.
2. Could there be a restriction on tweaking of max_parallel_workers while
the parallel workers are at play? At least do not allow setting the
max_parallel_workers less than the current # of active parallel workers.

Regards,
Neha

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Neha Khatri]

Looking further in this context, number of active parallel workers is:
parallel_register_count - parallel_terminate_count

Can active workers ever be greater than max_parallel_workers, I think no.
Then why should there be greater than check in the following condition:

if (parallel && (BackgroundWorkerData->parallel_register_count -
BackgroundWorkerData->parallel_terminate_count) >= max_parallel_workers)

I feel there should be an assert if
(BackgroundWorkerData->parallel_register_count
- BackgroundWorkerData->parallel_terminate_count) > max_parallel_workers)

And the check could be
if (parallel && (active_parallel_workers == max_parallel_workers))
then do not register new parallel wokers and return.

There should be no tolerance for the case when active_parallel_workers >
max_parallel_workers. After all that is the purpose of max_parallel_workers.

Is it like multiple backends trying to register parallel workers at the
same time, for which the greater than check should be present?

Thoughts?

Regards,
Neha

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Neha Khatri]

On Fri, Mar 31, 2017 at 8:29 AM, Kuntal Ghosh 
 wrote:

> On Fri, Mar 31, 2017 at 2:05 AM, Kuntal Ghosh
>  wrote:
> >
> > 1. Put an Assert(0) in ParallelQueryMain(), start server and execute
> > any parallel query.
> >  In LaunchParallelWorkers, you can see
> >nworkers = n nworkers_launched = n (n>0)
> > But, all the workers will crash because of the assert statement.
> > 2. the server restarts automatically, initialize
> > BackgroundWorkerData->parallel_register_count and
> > BackgroundWorkerData->parallel_terminate_count in the shared memory.
> > After that, it calls ForgetBackgroundWorker and it increments
> > parallel_terminate_count. In LaunchParallelWorkers, we have the
> > following condition:
> > if ((BackgroundWorkerData->parallel_register_count -
> >  BackgroundWorkerData->parallel_terminate_count) >=
> > max_parallel_workers)
> > DO NOT launch any parallel worker.
> > Hence, nworkers = n nworkers_launched = 0.
> parallel_register_count and parallel_terminate_count, both are
> unsigned integer. So, whenever the difference is negative, it'll be a
> well-defined unsigned integer and certainly much larger than
> max_parallel_workers. Hence, no workers will be launched. I've
> attached a patch to fix this.


The current explanation of active number of parallel workers is:

 * The active
 * number of parallel workers is the number of registered workers minus the
 * terminated ones.

In the situations like you mentioned above, this formula can give negative
number for active parallel workers. However a negative number for active
parallel workers does not make any sense.

I feel it would be better to explain in code that in what situations, the
formula
can generate a negative result and what that means.

Regards,
Neha

Re: [HACKERS] [NOVICE] Why is there a doubtful copyObject call in add_vars_to_targetlist

[Neha Khatri]

Sure, understood.

Regards,
Neha

Re: [HACKERS] [NOVICE] Why is there a doubtful copyObject call in add_vars_to_targetlist

[Neha Khatri]

On Mon, Mar 13, 2017 at 3:52 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> David Rowley <david.row...@2ndquadrant.com> writes:
> > On 13 March 2017 at 14:22, Neha Khatri <nehakhat...@gmail.com> wrote:
> >> This copyObject still exits in the current code. So I was wondering if
> the
> >> comment question still holds good and why the question there in first
> place.
> >> To make a new Var object, copyObject seem to be the right choice, then
> why
> >> the doubt?
>
> > The doubt is in the fact if copyObject() is required at all. The other
> > option being to simply reference the same object without having made a
> copy.
>
> Right.  Note that the code that 5efe3121 replaced effectively made a new
> Var object using makeVar.  The new code makes a new Var object using
> copyObject, so there's no actual behavioral change in that fragment, just
> fewer lines of code.  But it's fair to wonder whether it wouldn't be safe
> just to link to the existing Var object.  This is tied up in the planner's
> general willingness to scribble on its input data structures, so that
> linking to a pre-existing object is vulnerable to some unrelated bit of
> code deciding to scribble on that object.  Ideally that wouldn't happen
> ... but cleaning it up looks like a mighty tedious bit of janitorial work,
> with uncertain payoff.  So it hasn't happened in the last twenty years
> and I'm not prepared to bet that it ever will.
>

Then, should it be alright to remove the doubt itself?

Regards,
Neha

Re: [HACKERS] bytea_output vs make installcheck

[Neha Khatri]

On Fri, Mar 10, 2017 at 6:14 AM, Peter Eisentraut  wrote:

> On 2/14/17 16:50, Jeff Janes wrote:
> > make installcheck currently fails against a server running
> > with bytea_output = escape.
> >
> > Making it succeed is fairly easy, and I think it is worth doing.
> >
> > Attached are two options for doing that.  One overrides bytea_output
> > locally where-ever needed, and the other overrides it for the entire
> > 'regression' database.
>
> I would use option 2 here (ALTER DATABASE) and be done with it.  Some
> people didn't like using ALTER DATABASE, but it's consistent with
> existing use.  If someone wants to change that, that can be independent
> of this issue.


Sorry about the naive question, but if someone has set the GUC bytea_output
= 'escape', then the intention seem to be to obtain the output in 'escape'
format for bytea.
With this, if an installcheck is done, that might also have been done with
the expectation that the output will be in 'escape' format. In that case,
how much is it justified to hard code the format for regression database?
However, I agree that there are not many bytea outputs in the current
regression suite

Regards,
Neha

Re: [HACKERS] [NOVICE] opr_charset rule in gram.y

[Neha Khatri]

I see it is already addressed in master. Thanks.

Regards,
Neha

Re: [HACKERS] [NOVICE] opr_charset rule in gram.y

[Neha Khatri]

On Tue, Mar 7, 2017 at 4:30 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Neha Khatri <nehakhat...@gmail.com> writes:
> > I was going through the grammer rule for Character types in gram.y and
> > found an optional sub rule in is "opt_charset"
>
> This question seems quite off-topic for pgsql-novice, so I've redirected
> it to pgsql-hackers.
>

Thanks for appropriately categorizing. I was not sure.


> > CharacterWithLength:  character '(' Iconst ')' opt_charset
> > {
> > if (($5 != NULL) && (strcmp($5, "sql_text") != 0))
> > $1 = psprintf("%s_%s", $1, $5);
> >
> > $$ = SystemTypeName($1);
>
> > I would like to understand how opt_charset would get used in real
> > applications. I tried to make use of it but results in failure:
>
> > postgres=# create table testchar (c1 char(5) character set utf8);
> > ERROR:  type "pg_catalog.bpchar_utf8" does not exist
> > LINE 1: create table testchar (c1 char(5) character set utf8);
>
> > There does not seem to be any documentation available about this optional
> > parameter in the documents for Character data type( at least I could not
> > find any).
>
> Some digging in the git history shows that opt_charset was introduced in
> commit f10b63923 (in 1997!), and the current interpretation as
> "typename_charsetname" appeared in commit 3e1beda2c.  Neither commit
> adds any user documentation or even mentions fooling with character type
> declarations in its commit message.  I think that this must have been
> work-in-progress that Tom Lockhart left behind when he dropped out of
> the project circa 2002.
>
> Since there are no datatypes matching the "typename_charsetname" naming
> pattern, and today we'd be unlikely to accept an implementation based on
> that approach even if someone wrote one, my inclination is just to rip out
> this code.  Maybe we could leave behind the no-op case that allows
> "CHARACTER SET SQL_TEXT", but I don't really see the point, since we've
> never documented supporting any such syntax.
>
> Ok, understood, good historical detail.. Will go through
"typename_charsetname"
to understand the concept. Then may be come up with the cleanup patch.

Thanks and Regards,
Neha

Re: [HACKERS] [PATCH] SortSupport for macaddr type

[Neha Khatri]

Hi Brandur,

Couple of typo corrections required in patch:

s/converstion/conversion
s/go the heap/go to the heap

The performance results you shared are for he Index Creation operation.
Are there similar results for the sorting using ORDER BY queries too? Just
curious.

Regards,
Neha

[HACKERS] Resolved typo in a comment

[neha khatri]

Hi,

Attached is a patch that fixes a comment typo in varlena.c


Thanks,
Neha


typo_correction.patch
Description: Binary data

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

Re: [HACKERS] bytea_output vs make installcheck

[neha khatri]

Agreed with Jeff, false alarms should be avoided, whenever it is easy to
put the avoiding mechanism in place.

Regards,
Neha

Re: [HACKERS] bytea_output vs make installcheck

[neha khatri]

On Wed, Feb 15, 2017 at 10:04 AM, neha khatri <nehakhat...@gmail.com>
 wrote:.
>
>
>> Attached are two options for doing that.  One overrides bytea_output
>> locally where-ever needed, and the other overrides it for the entire
>> 'regression' database.
>>
>
> The solution that overrides bytea_output locally looks appropriate. It may
> not be required to change the format for entire database.
> Had there been a way to convert  bytea_output from 'hex' to 'escape'
> internally, that could have simplified this customization even more.
>

Well, the conversion from 'hex' to 'escape' is available using the function
encode().
So the queries that are failing due to the setting bytea_output =  escape,
can be wrapped under encode(), to obtain the result in 'escape' format.
Here is another way to resolve the same problem. The patch is attached.

Regards,
Neha


intallcheck_bytea_output_escape.patch
Description: Binary data

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

Re: [HACKERS] bytea_output vs make installcheck

[neha khatri]

On Wed, Feb 15, 2017 at 8:50 AM, Jeff Janes  wrote:

> make installcheck currently fails against a server running
> with bytea_output = escape.
>
> Making it succeed is fairly easy, and I think it is worth doing.
>
> Attached are two options for doing that.  One overrides bytea_output
> locally where-ever needed, and the other overrides it for the entire
> 'regression' database.
>

The solution that overrides bytea_output locally looks appropriate. It may
not be required to change the format for entire database.
Had there been a way to convert  bytea_output from 'hex' to 'escape'
internally, that could have simplified this customisation even more.

Regards,
Neha

Cheers,
Neha

On Wed, Feb 15, 2017 at 8:50 AM, Jeff Janes  wrote:

> make installcheck currently fails against a server running
> with bytea_output = escape.
>
> Making it succeed is fairly easy, and I think it is worth doing.
>
> Attached are two options for doing that.  One overrides bytea_output
> locally where-ever needed, and the other overrides it for the entire
> 'regression' database.
>
> Cheers,
>
> Jeff
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] Unsafe use of relation->rd_options without checking its type

[neha khatri]

>
>
>  ^
>
> The reason for the error is that transformOnConflictArbiter applies
> RelationIsUsedAsCatalogTable() to something that it doesn't know to
> be a plain relation --- it's a view in this case.  And that macro
> blindly assumes that relation->rd_options is a StdRdOptions struct,
> when in this case it's actually a ViewOptions struct.
>
> Now that I've seen this I wonder which other uses of rd_options are
> potentially broken.  RelationIsUsedAsCatalogTable() is hardly the
> only macro that is assuming with little justification that it's
> applied to the right kind of reloptions.
>

Right, there are many macros, which blindly assume the rd_options
to be of specific type. Here is the list of such macros :

 RelationGetFillFactor
 RelationIsUsedAsCatalogTable
 RelationGetParallelWorkers
 RelationIsSecurityView
 RelationHasCheckOption
 RelationHasLocalCheckOption
 RelationHasCascadedCheckOption
 BrinGetPagesPerRange
 GinGetUseFastUpdate
 GinGetPendingListCleanupSize

For the macros associated with specific index type, it might be alright to
assume the type of the rd_options because those have very limited usage.
However for the rest of the macros, the code does not limit its usage. This
can
lead to problems as you described above .



> We could band-aid this by having the RelationIsUsedAsCatalogTable()
> macro check relation->relkind, but I'm inclined to think that the
> right fix going forward is to insist that StdRdOptions, ViewOptions,
> and the other in-memory representations of reloptions ought to be
> self-identifying somehow.  We could add a field to them similar to
> nodeTag, but one of the things that was envisioned to begin with
> is relation types that have StdRdOptions as the first part of a
> larger struct.  I'm not sure how to make that work with a tag.
>
> Thoughts?
>

Yes, it seems appropriate to tag all types of the rd_options with an
identification and maybe check for that identification within each macro.
Currently, there are following types of rd_options as far as I could find:

 GiSTOptions
 BrinOptions
 GinOptions
 StdRdOptions
 ViewOptions
 BloomOptions (from contrib)

I am not clear on how the identification field in the above structures can
be a problem,  for the relations have StdRdOptions as the first part of a
larger structure.

Regards,
Neha

Re: [HACKERS] RFC: replace pg_stat_activity.waiting with something more descriptive

[neha khatri]

Hello,

I noticed that a small optimization is possible in the flow of wait stat
reporting for the LWLocks, when the pgstat_track_activities is disabled.
If the check for pgstat_track_activities is done before invoking
 LWLockReportWaitStart() instead of inside the pgstat_report_wait_start(),
it can save some of the statements execution where the end result of
LWLockReportWaitStart() is a NO-OP because pgstat_track_activities = false.

I also see, that there are other callers of pgstat_report_wait_start() as
well, which would also have to change to add a check for the
pg_stat_activities, if the check is removed from the
pgstat_report_wait_start(). Is the pg_stat_activities check inside
pgstat_report_wait_start() because of some protocol being followed? Would
it be worth making that change.

Regards,
Neha

Cheers,
Neha

On Thu, Mar 10, 2016 at 12:31 AM, Amit Kapila 
wrote:

> On Fri, Mar 4, 2016 at 7:11 PM, Alexander Korotkov 
> wrote:
> >
> >>
> >> If yes, then the only slight worry is that there will lot of repetition
> in wait_event_type column, otherwise it is okay.
> >
> >
> > There is morerows attribute of entry tag in Docbook SGML, it behaves
> like rowspan in HTML.
> >
>
> Thanks for the suggestion.  I have updated the patch to include
> wait_event_type information in the wait_event table.
>
> As asked above by Robert, below is performance data with the patch.
>
> M/C Details
> --
> IBM POWER-8 24 cores, 192 hardware threads
> RAM = 492GB
>
> Performance Data
> 
> min_wal_size=15GB
> max_wal_size=20GB
> checkpoint_timeout=15min
> maintenance_work_mem = 1GB
> checkpoint_completion_target = 0.9
>
>
> pgbench read-only (median of 3, 5-min runs)
>
> clients BASE PATCH %
> 1 19703.549206 19992.141542 1.4646718364
> 8 120105.542849 127717.835367 6.3380026745
> 64 487334.338764 495861.7211254 1.7498012521
>
>
> The read-only data shows some improvement with patch, but I think this is
> mostly attributed to run-to-run variation.
>
> pgbench read-write (median of 3, 30-min runs)
>
> clients BASE PATCH %
> 1 1703.275728 1696.568881 -0.3937616729
> 8 8884.406185 9442.387472 6.2804567394
> 64 32648.82798 32113.002416 -1.6411785572
>
>
> In the above data, the read-write data shows small regression (1.6%) at
> higher client-count, but when I ran individually that test, the difference
> was 0.5%. I think it is mostly attributed to run-to-run variation as we see
> with read-only tests.
>
>
> Thanks to Mithun C Y for doing performance testing of this patch.
>
> As this patch is adding 4-byte variable to shared memory structure PGPROC,
> so this is susceptible to memory alignment issues for shared buffers as
> discussed in thread [1], but in general the performance data doesn't
> indicate any regression.
>
> [1] - http://www.postgresql.org/message-id/CAA4eK1+ZeB8PMwwktf+
> 3brs0pt4ux6rs6aom0uip8c6shjw...@mail.gmail.com
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[neha khatri]

On Wed, Aug 24, 2016 at 10:07 AM, Gavin Flower <
gavinflo...@archidevsys.co.nz> wrote:

> On 24/08/16 12:02, neha khatri wrote:
>
>> >Andres Freund <and...@anarazel.de <mailto:and...@anarazel.de>> writes:
>> >> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> >> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane <t...@sss.pgh.pa.us > t...@sss.pgh.pa.us>> wrote:
>> >>>> I'm inclined to suggest you forget this approach and propose a single
>> >>>> counter for "SQL commands executed", which avoids all of the above
>> >>>> definitional problems.  People who need more detail than that are
>> >>>> probably best advised to look to contrib/pg_stat_statements, anyway.
>>
>> >>> I disagree.  I think SQL commands executed, lumping absolutely
>> >>> everything together, really isn't much use.
>>
>> >> I'm inclined to agree. I think that's a quite useful stat when looking
>> >> at an installation one previously didn't have a lot of interaction
>> with.
>>
>> >Well, let's at least have an "other" category so you can add up the
>> >counters and get a meaningful total.
>>
>> How would that meaningful total might help a user. What can a user might
>> analyse with the counter in 'other' category.
>>
>>
>>
>> The user could then judge if there were a significant number of examples
> not covered in the other categories - this may, or may not, be a problem;
> depending on the use case.
>
>
> Cheers,
> Gavin
>
> For the user to be able to judge that whether the number in the 'other'
category is a problem or not, the user is also required to know what all
might fall under the 'other' category. It may not be good to say that
_anything_ that is not part of the already defined category is part of
'other'. Probably, 'other' should also be a set of predefined operations.

Neha

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[neha khatri]

>Andres Freund  writes:
>> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:
 I'm inclined to suggest you forget this approach and propose a single
 counter for "SQL commands executed", which avoids all of the above
 definitional problems.  People who need more detail than that are
 probably best advised to look to contrib/pg_stat_statements, anyway.

>>> I disagree.  I think SQL commands executed, lumping absolutely
>>> everything together, really isn't much use.

>> I'm inclined to agree. I think that's a quite useful stat when looking
>> at an installation one previously didn't have a lot of interaction with.

>Well, let's at least have an "other" category so you can add up the
>counters and get a meaningful total.

How would that meaningful total might help a user. What can a user might
analyse with the counter in 'other' category.


Neha