Re: [HACKERS] Doubt in 9.5 release note

[Bruce Momjian]

On Wed, Jan 27, 2016 at 01:31:45PM +0900, Tatsuo Ishii wrote:
> > Tatsuo Ishii  writes:
> >> I saw following item in release-9.5.sgml:
> >> 
> >>
> >> Support comments on domain
> >> constraints (Álvaro Herrera)
> >>
> >>   
> > 
> >> It seems the release note has nothing to do with the commit. Also,
> >> commenting on DOMAIN constraints is not new in 9.5, I think.
> > 
> > Yeah, it is.  See 7eca575d1c28f6eee2bf4564f3d458d10c4a8f47, which is
> > the commit that should have been listed here, likely.
> 
> Are you willing to fix it?

Fixed and patched back to 9.5.

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

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


-- 
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] Doubt in 9.5 release note

[Tatsuo Ishii]

> Tatsuo Ishii  writes:
>> I saw following item in release-9.5.sgml:
>> 
>>
>> Support comments on domain
>> constraints (Álvaro Herrera)
>>
>>   
> 
>> It seems the release note has nothing to do with the commit. Also,
>> commenting on DOMAIN constraints is not new in 9.5, I think.
> 
> Yeah, it is.  See 7eca575d1c28f6eee2bf4564f3d458d10c4a8f47, which is
> the commit that should have been listed here, likely.

Are you willing to fix it?

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


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

Re: [HACKERS] Doubt in 9.5 release note

[Tom Lane]

Tatsuo Ishii  writes:
> I saw following item in release-9.5.sgml:
> 
>
> Support comments on domain
> constraints (Álvaro Herrera)
>
>   

> It seems the release note has nothing to do with the commit. Also,
> commenting on DOMAIN constraints is not new in 9.5, I think.

Yeah, it is.  See 7eca575d1c28f6eee2bf4564f3d458d10c4a8f47, which is
the commit that should have been listed here, likely.

regards, tom lane


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

[HACKERS] Doubt in 9.5 release note

[Tatsuo Ishii]

I saw following item in release-9.5.sgml:


   
Support comments on domain
constraints (Álvaro Herrera)
   
  

It seems the release note has nothing to do with the commit. Also,
commenting on DOMAIN constraints is not new in 9.5, I think.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

Hello Tatsuo,


So on second thought the formula should rather be:

  ...  / (is_connect? nclients: nthreads)


I don't think this is quite correct.

If is_connect is false, then following loop is executed in threadRun():

/* make connections to the database */
for (i = 0; i < nstate; i++)
{
if ((state[i].con = doConnect()) == NULL)
goto done;
}


Yep. The loop initializes all client connections *BEFORE* starting any 
transactions on any client, the thread does only do connections at this 
time, which is included conn_time.



Here, nstate is nclients/nthreads. Suppose nclients = 16 and nthreads
= 2, then 2 threads run in parallel, and each thread is connecting 8
times (nstate = 8) in *serial*.


Yes.

The total connection time for this thread is calculated by "the time 
ends the loop" - "the time starts the loop". So if the time to establish 
a connection is 1 second, the total connection time for a thread will be 
8 seconds. Thus grand total of connection time will be 2 * 8 = 16 
seconds.


Yes, 16 seconds in 2 threads, 8 seconds per thread of the "real time" of 
the test is spend in the connection, and no



If is_connect is true, following loop is executed.

/* send start up queries in async manner */
for (i = 0; i < nstate; i++)
{
CState *st = &state[i];
Command   **commands = sql_files[st->use_file];
int prev_ecnt = st->ecnt;

st->use_file = getrand(thread, 0, num_files - 1);
if (!doCustom(thread, st, &thread->conn_time, logfile, &aggs))

In the loop, exactly same thing happens as is_connect = false case. If
t = 1, total connection time will be same as is_connect = false case,
i.e. 16 seconds.


Without -C, 1 thread, 2 clients, if transactions take same time as 
connections:


 Client 1:  C-|TTT
 Client 2:  -C|TTT
<> connection time (initial loop in threadRun)
<> whole execution time
   <-> transaction time

The actual transaction time to consider on this drawing is whole time 
minus the connection time of the thread, which is serialised. It is not 
whole execution time minus connection time / 2 (number of clients), 
because of the '|' synchronisation (clients do not start before all other 
clients of the thread are connected).


With -C, the is no initial connection, the connections are managed within
doCustom, which is doing transaction processing asynchronously.

 Client 1:  CTCTCTCTCTCTCTCTCTCTCTCTCTCT-
 Client 2:  -CTCTCTCTCTCTCTCTCTCTCTCTCTCT
<---> whole execution time
<-->  measured connection time

While a client is connecting, the other client is performing its 
transaction in an asynchronous manner, so the measured connection time may 
be arbitrary close to the execution time, this was the bug you detected.



In summary, I see no reason to change the v1 patch.


I still think that my revised thinking is the right one... I hope that the 
above drawings make my thinking clearer. For me, the initial formula was 
the right one when not using -C, only the -C case need fixing.


--
Fabien.


--
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] Doubt in pgbench TPS number

[Tatsuo Ishii]

>> FWIW, I vote with Tatsuo-san.  Such a change will break comparability of
>> results with all previous versions, which means it's not something to do
>> in minor releases, even if we now believe the previous results were
>> somewhat bogus.  Arguing that it's "not a behavioral change" seems
>> quite loony to me: for most people, the TPS numbers are the only
>> interesting output of pgbench.
>> 
>> I think we should fix it in 9.5 and document it as an incompatible change.
> 
> Plus if 9.4 or earlier version of PostgreSQL user wants to use fixed
> version of pgbench, he/she can always use pgbench coming with 9.5 or
> later version of PostgreSQL.
> 
>> (Note: I've not read the patch, so this is not an opinion about its
>> correctness.)
> 
> As Fabien anayzed the problem was that pgbench simply uses wrong
> variable: nthreads (number of threads, specified by "-j" option)
> vs. nclients (number of clients, specified by "-c" option).
> 
> @@ -2616,7 +2616,7 @@ printResults(int ttype, int64 normal_xacts, int 
> nclients,
>   time_include = INSTR_TIME_GET_DOUBLE(total_time);
>   tps_include = normal_xacts / time_include;
>   tps_exclude = normal_xacts / (time_include -
> - 
> (INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));
> + 
> (INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
> 
> Here conn_total_time is the sum of time to establish connection to
> PostgreSQL. Since establishing connections to PostgreSQL is done in
> serial rather than in parallel, conn_total_time should have been
> divided by nclients.

Fix committed to master and 9.5 stable branches.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Tatsuo Ishii]

>> Here conn_total_time is the sum of time to establish connection to
>> PostgreSQL. Since establishing connections to PostgreSQL is done in
>> serial rather than in parallel, conn_total_time should have been
>> divided by nclients.
> 
> After some more thinking and looking again at the connection code, I
> revise slightly my diagnostic:
> 
>  - the amount of parallelism is "nclients", as discussed above, when
>  - reconnecting on each transaction (-C) because the connections are
>  - managed in parallel from doCustom,
> 
> * BUT *
> 
>  - if there is no reconnections (not -C) the connections are performed in
>  - threadRun in a sequential way, all clients wait for the connections of
>  - other clients in the same thread before starting processing
>  - transactions, so "nthreads" is the right amount of parallelism in this
>  - case.
> 
> So on second thought the formula should rather be:
> 
>   ...  / (is_connect? nclients: nthreads)

I don't think this is quite correct.

If is_connect is false, then following loop is executed in threadRun():

/* make connections to the database */
for (i = 0; i < nstate; i++)
{
if ((state[i].con = doConnect()) == NULL)
goto done;
}

Here, nstate is nclients/nthreads. Suppose nclients = 16 and nthreads
= 2, then 2 threads run in parallel, and each thread is connecting 8
times (nstate = 8) in *serial*. The total connection time for this
thread is calculated by "the time ends the loop" - "the time starts
the loop". So if the time to establish a connection is 1 second, the
total connection time for a thread will be 8 seconds. Thus grand total
of connection time will be 2 * 8 = 16 seconds.

If is_connect is true, following loop is executed.

/* send start up queries in async manner */
for (i = 0; i < nstate; i++)
{
CState *st = &state[i];
Command   **commands = sql_files[st->use_file];
int prev_ecnt = st->ecnt;

st->use_file = getrand(thread, 0, num_files - 1);
if (!doCustom(thread, st, &thread->conn_time, logfile, &aggs))

In the loop, exactly same thing happens as is_connect = false case. If
t = 1, total connection time will be same as is_connect = false case,
i.e. 16 seconds.

In summary, I see no reason to change the v1 patch.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

(Note: I've not read the patch, so this is not an opinion about its
correctness.)


As Fabien anayzed the problem was that pgbench simply uses wrong
variable: nthreads (number of threads, specified by "-j" option)
vs. nclients (number of clients, specified by "-c" option).

@@ -2616,7 +2616,7 @@ printResults(int ttype, int64 normal_xacts, int nclients,
time_include = INSTR_TIME_GET_DOUBLE(total_time);
tps_include = normal_xacts / time_include;
tps_exclude = normal_xacts / (time_include -
-   
(INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));
+   
(INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));

Here conn_total_time is the sum of time to establish connection to
PostgreSQL. Since establishing connections to PostgreSQL is done in
serial rather than in parallel, conn_total_time should have been
divided by nclients.


After some more thinking and looking again at the connection code, I 
revise slightly my diagnostic:


 - the amount of parallelism is "nclients", as discussed above, when 
reconnecting on each transaction (-C) because the connections are managed 
in parallel from doCustom,


* BUT *

 - if there is no reconnections (not -C) the connections are performed in 
threadRun in a sequential way, all clients wait for the connections of 
other clients in the same thread before starting processing transactions, 
so "nthreads" is the right amount of parallelism in this case.


So on second thought the formula should rather be:

  ...  / (is_connect? nclients: nthreads)

Here is a v2 with this formula. Note that it does not take into account 
thread creation time, which might be significant, the start and end of a 
pgbench run are quite fuzzy.


I've removed the doCustom parameter change as it is included in a larger 
patch I submitted about reworking stat collections in pgbench, so this 
attached patch is bug fix only.


For the record, I still plainly disagree with the idea of shipping a 
performance measuring tool which knowingly displays wrong and optimistic 
figures under some conditions.


--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 6ae1b86..a6763f6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2616,7 +2616,8 @@ printResults(int ttype, int64 normal_xacts, int nclients,
 	time_include = INSTR_TIME_GET_DOUBLE(total_time);
 	tps_include = normal_xacts / time_include;
 	tps_exclude = normal_xacts / (time_include -
-		(INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));
+		(INSTR_TIME_GET_DOUBLE(conn_total_time) /
+		 (is_connect? nclients: nthreads)));
 
 	if (ttype == 0)
 		s = "TPC-B (sort of)";
@@ -3462,7 +3463,7 @@ main(int argc, char **argv)
 
 		/* thread level stats */
 		throttle_lag += thread->throttle_lag;
-		throttle_latency_skipped += threads->throttle_latency_skipped;
+		throttle_latency_skipped += thread->throttle_latency_skipped;
 		latency_late += thread->latency_late;
 		if (throttle_lag_max > thread->throttle_lag_max)
 			throttle_lag_max = thread->throttle_lag_max;

-- 
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] Doubt in pgbench TPS number

[Tatsuo Ishii]

> FWIW, I vote with Tatsuo-san.  Such a change will break comparability of
> results with all previous versions, which means it's not something to do
> in minor releases, even if we now believe the previous results were
> somewhat bogus.  Arguing that it's "not a behavioral change" seems
> quite loony to me: for most people, the TPS numbers are the only
> interesting output of pgbench.
> 
> I think we should fix it in 9.5 and document it as an incompatible change.

Plus if 9.4 or earlier version of PostgreSQL user wants to use fixed
version of pgbench, he/she can always use pgbench coming with 9.5 or
later version of PostgreSQL.

> (Note: I've not read the patch, so this is not an opinion about its
> correctness.)

As Fabien anayzed the problem was that pgbench simply uses wrong
variable: nthreads (number of threads, specified by "-j" option)
vs. nclients (number of clients, specified by "-c" option).

@@ -2616,7 +2616,7 @@ printResults(int ttype, int64 normal_xacts, int nclients,
time_include = INSTR_TIME_GET_DOUBLE(total_time);
tps_include = normal_xacts / time_include;
tps_exclude = normal_xacts / (time_include -
-   
(INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));
+   
(INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));

Here conn_total_time is the sum of time to establish connection to
PostgreSQL. Since establishing connections to PostgreSQL is done in
serial rather than in parallel, conn_total_time should have been
divided by nclients.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

Hello Tom,


FWIW, I vote with Tatsuo-san.  Such a change will break comparability of
results


I would not classify a performance measure as a "result compatibility" 
issue. What matters is the *correction* of the results.


When a bug is fixed anywhere in pg it may change performance significantly 
for some tests, and I have never seen this as a relevant consideration not 
to fix a problem...



with all previous versions, which means it's not something to do
in minor releases, even if we now believe the previous results were
somewhat bogus.  Arguing that it's "not a behavioral change" seems
quite loony to me: for most people, the TPS numbers are the only
interesting output of pgbench.


I think that if people are interested in tps without reconnecting on each 
transaction they would not run with "-C" to trigger reconnections and then 
look at the "tps without connections" for the figure they want... so I do 
not think that keeping this error is worth anything.


On the other hand, and on the principle, keeping the bug looks wrong. I 
cannot agree with the logic behind shipping something which is known bad, 
especially to display *optimistic* performances... It looks too much like 
the VW way:-)


I think we should fix it in 9.5 and document it as an incompatible 
change.


Hmm.


(Note: I've not read the patch, so this is not an opinion about its
correctness.)


That is another question:-)

--
Fabien.


--
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] Doubt in pgbench TPS number

[Tom Lane]

Fabien COELHO  writes:
>> Yeah, that's definitely a bug but I'm afraid the fix will change the
>> TPS number and may break the backward compatibility. Since we have
>> lived with bug for years, I hesitate to back port to older stable
>> branches...

> My 2¥: I do not think of a good argument to keep wrong tps numbers once it 
> is known that there are plain wrong, especially as it is not a behavioral 
> change as such which could block applications or whatever, just a 
> different number printed at the end of a run. So I would not bother much 
> with upward compatibility consideration in this case.

FWIW, I vote with Tatsuo-san.  Such a change will break comparability of
results with all previous versions, which means it's not something to do
in minor releases, even if we now believe the previous results were
somewhat bogus.  Arguing that it's "not a behavioral change" seems
quite loony to me: for most people, the TPS numbers are the only
interesting output of pgbench.

I think we should fix it in 9.5 and document it as an incompatible change.

(Note: I've not read the patch, so this is not an opinion about its
correctness.)

regards, tom lane


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

Re: [HACKERS] Doubt in pgbench TPS number

[Tatsuo Ishii]

>> Yeah, that's definitely a bug but I'm afraid the fix will change the
>> TPS number and may break the backward compatibility. Since we have
>> lived with bug for years, I hesitate to back port to older stable
>> branches...
> 
> My 2¥: I do not think of a good argument to keep wrong tps numbers
> once it is known that there are plain wrong, especially as it is not a
> behavioral change as such which could block applications or whatever,
> just a different number printed at the end of a run. So I would not
> bother much with upward compatibility consideration in this case.
> 
> -- 
> Fabien.

FYI, the bug was there since the thread support was introduced in commit:

da0dfb4b1460c3701abc8ed5f516d138dc4654c

That was in 9.0, which was released in 2010.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

#5  0x00402b2b in doConnect () at pgbench.c:650
#6  0x00404591 in doCustom (thread=0x25c2f40, st=0x25c2770,
   conn_time=0x25c2f90, logfile=0x0, agg=0x7fffba224260) at pgbench.c:1353
#7  0x0040a1d5 in threadRun (arg=0x25c2f40) at pgbench.c:3581
#8  0x00409ab4 in main (argc=12, argv=0x7fffba224668) at pgbench.c:3455


Hmm. Ok, whatever the connection position (from doCustom or from 
threadRun), doConnect would block the thread. On the other hand, if you 
start several threads, probably those threads which could connect all 
their client would proceed.


What looks to be needed would be some timeout when connecting to the 
server.



The formula change, and just this one, should probably be backported
somehow, as this is a bug, wherever pgbench resides in older
versions. It is just 's/nthreads/nclients/' in the printResult formula
for computing tps_exclude.


Yeah, that's definitely a bug but I'm afraid the fix will change the
TPS number and may break the backward compatibility. Since we have
lived with bug for years, I hesitate to back port to older stable
branches...


My 2¥: I do not think of a good argument to keep wrong tps numbers once it 
is known that there are plain wrong, especially as it is not a behavioral 
change as such which could block applications or whatever, just a 
different number printed at the end of a run. So I would not bother much 
with upward compatibility consideration in this case.


--
Fabien.
--
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] Doubt in pgbench TPS number

[Tatsuo Ishii]

>> I'm not sure about this. I think pgbench will not start transactions
>> until all clients establish connections to PostgreSQL.
> 
> I think that is true if "!is_connect", all client connections are
> performed at the beginning of threadRun, but under -C each client has
> its own connect/deconnect integrated within doCustom, so it is done in
> parallel to other clients having their transactions processed, hence
> the issue with the formula.

Really?

I have tested with pgpool-II which is set to accept up to 2
connections, then run pgbench with -C and -c 32. pgbench was blocked
as expected and I attached gdb and got stack trace:

(gdb) bt
#0  0x7f48d5f17110 in __poll_nocancel ()
at ../sysdeps/unix/syscall-template.S:81
#1  0x7f48d6724056 in pqSocketCheck ()
   from /usr/local/src/pgsql/current/lib/libpq.so.5
#2  0x7f48d6724940 in pqWaitTimed ()
   from /usr/local/src/pgsql/current/lib/libpq.so.5
#3  0x7f48d671f3e2 in connectDBComplete ()
   from /usr/local/src/pgsql/current/lib/libpq.so.5
#4  0x7f48d671fbcf in PQconnectdbParams ()
   from /usr/local/src/pgsql/current/lib/libpq.so.5
#5  0x00402b2b in doConnect () at pgbench.c:650
#6  0x00404591 in doCustom (thread=0x25c2f40, st=0x25c2770, 
conn_time=0x25c2f90, logfile=0x0, agg=0x7fffba224260) at pgbench.c:1353
#7  0x0040a1d5 in threadRun (arg=0x25c2f40) at pgbench.c:3581
#8  0x00409ab4 in main (argc=12, argv=0x7fffba224668) at pgbench.c:3455

As you can see, one of threads wants to connect to PostgreSQL
(actually pgpool-II) and waits for reply.

In threadRun() around line 3581:

/* send start up queries in async manner */
for (i = 0; i < nstate; i++)
{
CState *st = &state[i];
Command   **commands = sql_files[st->use_file];
int prev_ecnt = st->ecnt;

st->use_file = getrand(thread, 0, num_files - 1);
if (!doCustom(thread, st, &thread->conn_time, logfile, &aggs))
remains--;  /* I've aborted */

if (st->ecnt > prev_ecnt && commands[st->state]->type == 
META_COMMAND)
{
fprintf(stderr, "client %d aborted in state %d; 
execution of meta-command failed\n",
i, st->state);
remains--;  /* I've aborted */
PQfinish(st->con);
st->con = NULL;
}
}

Here doCustome() is called with st->con == NULL. In doCustom() around
line 1353:

if (st->con == NULL)
{
instr_time  start,
end;

INSTR_TIME_SET_CURRENT(start);
if ((st->con = doConnect()) == NULL)
{

doConnect() blocks until PostgreSQL (pgpool-II) allows to be
connected.

Because outer loop in threadRun() wants to loop over until all threads
succefully connects to PostgreSQL, pgbench is blocked here.

/* send start up queries in async manner */
for (i = 0; i < nstate; i++)

>> I'm going to commit your patch if there's no objection.
> 
> This seems fine with me.
> 
> The formula change, and just this one, should probably be backported
> somehow, as this is a bug, wherever pgbench resides in older
> versions. It is just 's/nthreads/nclients/' in the printResult formula
> for computing tps_exclude.

Yeah, that's definitely a bug but I'm afraid the fix will change the
TPS number and may break the backward compatibility. Since we have
lived with bug for years, I hesitate to back port to older stable
branches...

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

Hello Tatsuo-san,


I think that the degree of parallelism to consider is nclients, not
nthreads: while connection time is accumulated in conn_time, other
clients are possibly doing their transactions, in parallel,


I'm not sure about this. I think pgbench will not start transactions
until all clients establish connections to PostgreSQL.


I think that is true if "!is_connect", all client connections are 
performed at the beginning of threadRun, but under -C each client has its 
own connect/deconnect integrated within doCustom, so it is done in 
parallel to other clients having their transactions processed, hence the 
issue with the formula.



I found this while playing with pgpool-II. pgpool-II pre-forks child
process, whose number is defined by "num_init_children"
directive. What I observed was, pgbench starts connecting to pgpool-II
until "-c" connections are established. So, if "-c" is larger than
"num_init_children", no transaction starts.


Yep, unless -C, I think, where some client may start nevertheless? Not 
sure... Does not matter.



I have tested your patch. It seems the tolerance is much better than
before with your patch.


Yep.


I'm going to commit your patch if there's no objection.


This seems fine with me.

The formula change, and just this one, should probably be backported 
somehow, as this is a bug, wherever pgbench resides in older versions. It 
is just 's/nthreads/nclients/' in the printResult formula for computing 
tps_exclude.


--
Fabien.


--
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] Doubt in pgbench TPS number

[Tatsuo Ishii]

> I have tested your patch. It seems the tolerance is much better than
> before with your patch.

[snip]

> I'm going to commit your patch if there's no objection.

I think we should commit this to master and 9.5 stable branch only
because the fix significantly changes the benchmark result of pgbench.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Tatsuo Ishii]

> I think that the degree of parallelism to consider is nclients, not
> nthreads: while connection time is accumulated in conn_time, other
> clients are possibly doing their transactions, in parallel, 

I'm not sure about this. I think pgbench will not start transactions
until all clients establish connections to PostgreSQL.

I found this while playing with pgpool-II. pgpool-II pre-forks child
process, whose number is defined by "num_init_children"
directive. What I observed was, pgbench starts connecting to pgpool-II
until "-c" connections are established. So, if "-c" is larger than
"num_init_children", no transaction starts.

> even if it
> is in the same thread, so it is not "stopped time" for all clients. It
> starts to matter with "-j 1 -c 30" and slow transactions, the
> cumulated conn_time in each thread may be arbitrary close to the whole
> time if there are many clients.
> 
> Now, I do not think that this tps computation makes that much
> sense. If you want to know the tps without reconnect, run without
> reconnecting... It is clear that I do not get this figure when running
> without -C, so maybe
> the tps with/without reconnection should be dropped?
> 
> Anyway, here is a patch, which:
>  - fixes the "exclude" computation (s/nthreads/nclients/)
>  - fixes the count total for skipped (s/threads/thread/)
>  - removes a useless parameter to doCustom
>(the conn_time is available through the thread parameter).

I have tested your patch. It seems the tolerance is much better than
before with your patch.

With the patch:
./pgbench -C -n -p 11002 -c 10 -T 30 -f test.sql  test
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 30 s
number of transactions actually processed: 2887
latency average: 103.914 ms
tps = 95.896850 (including connections establishing)
tps = 98.101662 (excluding connections establishing)

Without the patch:
./pgbench -C -n -p 11002 -c 10 -T 30 -f test.sql  test
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 30 s
number of transactions actually processed: 2887
latency average: 103.914 ms
tps = 95.919415 (including connections establishing)
tps = 124.732475 (excluding connections establishing)

I'm going to commit your patch if there's no objection.

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


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

Re: [HACKERS] Doubt in pgbench TPS number

[Fabien COELHO]

Hello Tatsuo,


Hmmm... I never use -C. The formula seems ok:

   tps_exclude = normal_xacts / (time_include -
   (INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));


Hmmm... it is not:-)

I think that the degree of parallelism to consider is nclients, not 
nthreads: while connection time is accumulated in conn_time, other clients 
are possibly doing their transactions, in parallel, even if it is in the 
same thread, so it is not "stopped time" for all clients. It starts to 
matter with "-j 1 -c 30" and slow transactions, the cumulated conn_time in 
each thread may be arbitrary close to the whole time if there are many 
clients.


Now, I do not think that this tps computation makes that much sense. If 
you want to know the tps without reconnect, run without reconnecting... It 
is clear that I do not get this figure when running without -C, so maybe

the tps with/without reconnection should be dropped?

Anyway, here is a patch, which:
 - fixes the "exclude" computation (s/nthreads/nclients/)
 - fixes the count total for skipped (s/threads/thread/)
 - removes a useless parameter to doCustom
   (the conn_time is available through the thread parameter).

--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 6ae1b86..9a7dd6a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1126,7 +1126,7 @@ agg_vals_init(AggVals *aggs, instr_time start)
 
 /* return false iff client should be disconnected */
 static bool
-doCustom(TState *thread, CState *st, instr_time *conn_time, FILE *logfile, AggVals *agg)
+doCustom(TState *thread, CState *st, FILE *logfile, AggVals *agg)
 {
 	PGresult   *res;
 	Command   **commands;
@@ -1357,7 +1357,7 @@ top:
 			return clientDone(st, false);
 		}
 		INSTR_TIME_SET_CURRENT(end);
-		INSTR_TIME_ACCUM_DIFF(*conn_time, end, start);
+		INSTR_TIME_ACCUM_DIFF(thread->conn_time, end, start);
 	}
 
 	/*
@@ -2616,7 +2616,7 @@ printResults(int ttype, int64 normal_xacts, int nclients,
 	time_include = INSTR_TIME_GET_DOUBLE(total_time);
 	tps_include = normal_xacts / time_include;
 	tps_exclude = normal_xacts / (time_include -
-		(INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));
+		(INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
 
 	if (ttype == 0)
 		s = "TPC-B (sort of)";
@@ -3462,7 +3462,7 @@ main(int argc, char **argv)
 
 		/* thread level stats */
 		throttle_lag += thread->throttle_lag;
-		throttle_latency_skipped += threads->throttle_latency_skipped;
+		throttle_latency_skipped += thread->throttle_latency_skipped;
 		latency_late += thread->latency_late;
 		if (throttle_lag_max > thread->throttle_lag_max)
 			throttle_lag_max = thread->throttle_lag_max;
@@ -3578,7 +3578,7 @@ threadRun(void *arg)
 		int			prev_ecnt = st->ecnt;
 
 		st->use_file = getrand(thread, 0, num_files - 1);
-		if (!doCustom(thread, st, &thread->conn_time, logfile, &aggs))
+		if (!doCustom(thread, st, logfile, &aggs))
 			remains--;			/* I've aborted */
 
 		if (st->ecnt > prev_ecnt && commands[st->state]->type == META_COMMAND)
@@ -3717,7 +3717,7 @@ threadRun(void *arg)
 			if (st->con && (FD_ISSET(PQsocket(st->con), &input_mask)
 			|| commands[st->state]->type == META_COMMAND))
 			{
-if (!doCustom(thread, st, &thread->conn_time, logfile, &aggs))
+if (!doCustom(thread, st, logfile, &aggs))
 	remains--;	/* I've aborted */
 			}
 

-- 
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] Doubt in pgbench TPS number

[Fabien COELHO]

Hello Tatsuo-san,


$ pgbench -C -n -p 11002 -c 10 -T 30 -f test.sql  test
tps = 95.799114 (including connections establishing)
tps = 124.487149 (excluding connections establishing)

Interesting thing is, the number from "excluding connections
establishing". 124.487149 tps means 0.008032 seconds per
transaction. Since the query executes pg_sleep(0.1), I think the
number should be equal to or greater than 0.1. Maybe a tolerance, but
20% of error seems to be too high for me.


Indeed.


Note that if "-C" does not present, the TPS number seems to be sane.


Hmmm... I never use -C. The formula seems ok:

tps_exclude = normal_xacts / (time_include -
(INSTR_TIME_GET_DOUBLE(conn_total_time) / nthreads));

conn_total_time is the cumulated time spent by all threads.

A quick look at the logic reveals some minor issues (conn_time is passed 
as an argument to doCustom, although it is already available as a "thread" 
field, stange). I spotted "threads" used instead of "thread" in an 
accumulation, but it is not related to this computation.


Hmmm. I'll have a slower look...

--
Fabien.


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

[HACKERS] Doubt in pgbench TPS number

[Tatsuo Ishii]

Today I got an interesting output from pgbench.
The scenario is executing SELECT pg_sleep(0.1).

$ cat test.sql 
select pg_sleep(0.1);

$ pgbench -C -n -p 11002 -c 10 -T 30 -f test.sql  test
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 30 s
number of transactions actually processed: 2883
latency average: 104.058 ms
tps = 95.799114 (including connections establishing)
tps = 124.487149 (excluding connections establishing)

Interesting thing is, the number from "excluding connections
establishing". 124.487149 tps means 0.008032 seconds per
transaction. Since the query executes pg_sleep(0.1), I think the
number should be equal to or greater than 0.1. Maybe a tolerance, but
20% of error seems to be too high for me.

Note that if "-C" does not present, the TPS number seems to be sane.

$ pgbench -n -p 11002 -c 10 -T 30 -f test.sql  test
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 30 s
number of transactions actually processed: 2970
latency average: 101.010 ms
tps = 98.692514 (including connections establishing)
tps = 98.747053 (excluding connections establishing)
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Wed, Aug 5, 2015 at 9:21 PM, Michael Paquier 
wrote:
>
> On Thu, Aug 6, 2015 at 3:06 AM, Fabrízio de Royes Mello wrote:
> > On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas wrote:
> >> Agreed.  I think we're making a mountain out of a molehill here.  As
> >> long as the locks that are actually used are monotonic, just use > and
> >> stick a comment in there explaining that it could need adjustment if
> >> we use other lock levels in the future.  I presume all the lock-levels
> >> used for DDL are, and will always be, self-exclusive, so why all this
> >> hand-wringing?
> >>
> >
> > New version attached with suggested changes.
>
> Thanks!
>
> +# SET autovacuum_* options needs a ShareUpdateExclusiveLock
> +# so we mix reads with it to see what works or waits
> s/needs/need/ and I think you mean mixing "writes", not "reads".
>
> Those are minor things though, and from my point of view a committer
> can look at it.
>

Fixed. Thanks for your review.

Regards,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_analyze_threshold",
 			"Minimum number of tuple inserts, updates or deletes prior to analyze",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_cost_delay",
 			"Vacuum cost delay in milliseconds, for aut

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Thu, Aug 6, 2015 at 3:06 AM, Fabrízio de Royes Mello wrote:
> On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas wrote:
>> Agreed.  I think we're making a mountain out of a molehill here.  As
>> long as the locks that are actually used are monotonic, just use > and
>> stick a comment in there explaining that it could need adjustment if
>> we use other lock levels in the future.  I presume all the lock-levels
>> used for DDL are, and will always be, self-exclusive, so why all this
>> hand-wringing?
>>
>
> New version attached with suggested changes.

Thanks!

+# SET autovacuum_* options needs a ShareUpdateExclusiveLock
+# so we mix reads with it to see what works or waits
s/needs/need/ and I think you mean mixing "writes", not "reads".

Those are minor things though, and from my point of view a committer
can look at it.
Regards,
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas  wrote:
>
> On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera 
wrote:
> > That opens up for lock escalation and deadlocks, doesn't it?  You are
> > probably thinking that it's okay to ignore those but I don't necessarily
> > agree with that.
>
> Agreed.  I think we're making a mountain out of a molehill here.  As
> long as the locks that are actually used are monotonic, just use > and
> stick a comment in there explaining that it could need adjustment if
> we use other lock levels in the future.  I presume all the lock-levels
> used for DDL are, and will always be, self-exclusive, so why all this
> hand-wringing?
>

New version attached with suggested changes.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_analyze_threshold",
 			"Minimum number of tuple inserts, updates or deletes prior to analyze",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_cost_delay",
 			"Vacuum cost delay in milliseconds, for autovacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, 100
 	},
@@ -159,7 +171,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_cost_limit",
 			"Vacuum cost amount available before napping, for autovacuum",
-			RELOPT_KIND

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Robert Haas]

On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera  wrote:
> That opens up for lock escalation and deadlocks, doesn't it?  You are
> probably thinking that it's okay to ignore those but I don't necessarily
> agree with that.

Agreed.  I think we're making a mountain out of a molehill here.  As
long as the locks that are actually used are monotonic, just use > and
stick a comment in there explaining that it could need adjustment if
we use other lock levels in the future.  I presume all the lock-levels
used for DDL are, and will always be, self-exclusive, so why all this
hand-wringing?

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


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Wed, Aug 5, 2015 at 2:15 AM, Alvaro Herrera  wrote:
> Andres Freund wrote:
>> On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
>
>> > As long as this only applies on master, this may be fine... We could
>> > basically pass a LOCKMASK to the multiple layers of tablecmds.c
>> > instead of LOCKMODE to track all the locks that need to be taken, and
>> > all the relations open during operations.
>>
>> This sounds far too complicated to me. Just LockRelationOid() the
>> relation with the appropriate level everytime you pass through the
>> function?
>
> That opens up for lock escalation and deadlocks, doesn't it?  You are
> probably thinking that it's okay to ignore those but I don't necessarily
> agree with that.

Yes, I think so as long as we would try to take multiple locks... And
we go back to the lock hierarchy then, because there is no way to
define in some cases which lock is strictly stronger than the other.
Honestly I think that we should go with the version Fabrizio doing the
lock comparison, with a assert safeguard in AlterTableGetLockLevel.
The logic would get broken if ALTER TABLE begins to use a lock level
that cannot be ordered according to the others, but that's not the
case now.
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Alvaro Herrera]

Andres Freund wrote:
> On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:

> > As long as this only applies on master, this may be fine... We could
> > basically pass a LOCKMASK to the multiple layers of tablecmds.c
> > instead of LOCKMODE to track all the locks that need to be taken, and
> > all the relations open during operations.
> 
> This sounds far too complicated to me. Just LockRelationOid() the
> relation with the appropriate level everytime you pass through the
> function?

That opens up for lock escalation and deadlocks, doesn't it?  You are
probably thinking that it's okay to ignore those but I don't necessarily
agree with that.

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


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

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Tue, Aug 4, 2015 at 5:55 AM, Andres Freund  wrote:
>
> On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
> > On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> > > On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
> > >>> For instance, if you told me to choose between ShareLock and
> > >>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> > >>> don't it's sensible to have the "lock mode compare" primitive
> > >>honestly.
> > >>> I don't have any great ideas to offer ATM sadly.
> > >>
> > >>Yes, the thing is that lowering the lock levels is good for
> > >>concurrency, but the non-monotony of the lock levels makes it
> > >>impossible to choose an intermediate state correctly.
> > >
> > > How about simply acquiring all the locks individually of they're
different types? These few acquisitions won't matter.
> >
> > As long as this only applies on master, this may be fine... We could
> > basically pass a LOCKMASK to the multiple layers of tablecmds.c
> > instead of LOCKMODE to track all the locks that need to be taken, and
> > all the relations open during operations.
>
> This sounds far too complicated to me. Just LockRelationOid() the
> relation with the appropriate level everytime you pass through the
> function?

Hi all,

IMHO is more simply we just fallback to AccessExclusiveLock if there are
different lockmodes in reloptions as Michael suggested before.

Look at the new version attached.

Regards,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..e0f9f09 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KI

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Andres Freund]

On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
> On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> > On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
> >>> For instance, if you told me to choose between ShareLock and
> >>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> >>> don't it's sensible to have the "lock mode compare" primitive
> >>honestly.
> >>> I don't have any great ideas to offer ATM sadly.
> >>
> >>Yes, the thing is that lowering the lock levels is good for
> >>concurrency, but the non-monotony of the lock levels makes it
> >>impossible to choose an intermediate state correctly.
> >
> > How about simply acquiring all the locks individually of they're different 
> > types? These few acquisitions won't matter.
> 
> As long as this only applies on master, this may be fine... We could
> basically pass a LOCKMASK to the multiple layers of tablecmds.c
> instead of LOCKMODE to track all the locks that need to be taken, and
> all the relations open during operations.

This sounds far too complicated to me. Just LockRelationOid() the
relation with the appropriate level everytime you pass through the
function?


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Tue, Aug 4, 2015 at 9:12 AM, Fabrízio de Royes Mello
 wrote:
> Are you sure we need to do all this changes just to check the highest
> locklevel based on the reloptions?

Well, by looking at the code that's what it looks as. That's a large
change not that straight-forward.

> Or I misunderstood the whole thing?

No I think we are on the same page.

> Maybe we just check the DoLockModeConflict in the new method
> GetReloptionsLockLevel and if true then fallback to AccessExclusiveLock (as
> Michael suggested in a previous email).

Honestly that's what I would suggest for this patch, and also fix the
existing problem of tablecmds.c that does the same assumption. It
seems saner to me for now than adding a whole new level of routines
and wrappers, and your patch has IMO great value when each ALTER
COMMAND is kicked individually.
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Mon, Aug 3, 2015 at 2:15 AM, Michael Paquier 
wrote:
>
> On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> > On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
> >>> For instance, if you told me to choose between ShareLock and
> >>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> >>> don't it's sensible to have the "lock mode compare" primitive
> >>honestly.
> >>> I don't have any great ideas to offer ATM sadly.
> >>
> >>Yes, the thing is that lowering the lock levels is good for
> >>concurrency, but the non-monotony of the lock levels makes it
> >>impossible to choose an intermediate state correctly.
> >
> > How about simply acquiring all the locks individually of they're
different types? These few acquisitions won't matter.
>
> As long as this only applies on master, this may be fine... We could
> basically pass a LOCKMASK to the multiple layers of tablecmds.c
> instead of LOCKMODE to track all the locks that need to be taken, and
> all the relations open during operations. Would it be worth having
> some routines like relation_multi_[open|close]() as well? Like that:
> Relation[] relation_multi_open(relation Oid, LOCKMASK):
> void relation_multi_close(Relation[]);
>
> If we do something, we may consider patching as well 9.5, it seems to
> me that tablecmds.c is broken by assuming that lock hierarchy is
> monotone in AlterTableGetLockLevel().
>

Hey guys,

Are you sure we need to do all this changes just to check the highest
locklevel based on the reloptions? Or I misunderstood the whole thing?

Maybe we just check the DoLockModeConflict in the new method
GetReloptionsLockLevel and if true then fallback to AccessExclusiveLock (as
Michael suggested in a previous email).


Regards,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
>>> For instance, if you told me to choose between ShareLock and
>>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
>>> don't it's sensible to have the "lock mode compare" primitive
>>honestly.
>>> I don't have any great ideas to offer ATM sadly.
>>
>>Yes, the thing is that lowering the lock levels is good for
>>concurrency, but the non-monotony of the lock levels makes it
>>impossible to choose an intermediate state correctly.
>
> How about simply acquiring all the locks individually of they're different 
> types? These few acquisitions won't matter.

As long as this only applies on master, this may be fine... We could
basically pass a LOCKMASK to the multiple layers of tablecmds.c
instead of LOCKMODE to track all the locks that need to be taken, and
all the relations open during operations. Would it be worth having
some routines like relation_multi_[open|close]() as well? Like that:
Relation[] relation_multi_open(relation Oid, LOCKMASK):
void relation_multi_close(Relation[]);

If we do something, we may consider patching as well 9.5, it seems to
me that tablecmds.c is broken by assuming that lock hierarchy is
monotone in AlterTableGetLockLevel().
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Andres Freund]

On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier 
 wrote:
>On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera
> wrote:
>> Fabrízio de Royes Mello wrote:
>>
>>> In this patch I didn't change all lockmode comparison places
>previous
>>> pointed by you, but I can change it maybe adding other method called
>>> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock
>&&
>>> lockmode < MAX_LOCKMODES" used in many places.
>>
>> I don't like this.  Is it possible to write these comparisons in
>terms
>> of what they conflict with?  I think there are two main cases in the
>> existing code:
>>
>> 1. "is this lock mode valid" (sounds reasonable)
>> 2. "can this be acquired in hot standby" (not so much, but makes
>>sense.)
>>
>> and now we have your third thing, "what is the strongest of these two
>> locks".
>
>The third method already exists in tablecmds.c:
>/*
> * Take the greatest lockmode from any subcommand
> */
>if (cmd_lockmode > lockmode)
>lockmode = cmd_lockmode;
>
>> For instance, if you told me to choose between ShareLock and
>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
>> don't it's sensible to have the "lock mode compare" primitive
>honestly.
>> I don't have any great ideas to offer ATM sadly.
>
>Yes, the thing is that lowering the lock levels is good for
>concurrency, but the non-monotony of the lock levels makes it
>impossible to choose an intermediate state correctly. 

How about simply acquiring all the locks individually of they're different 
types? These few acquisitions won't matter.

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera  wrote:
> Fabrízio de Royes Mello wrote:
>
>> In this patch I didn't change all lockmode comparison places previous
>> pointed by you, but I can change it maybe adding other method called
>> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock &&
>> lockmode < MAX_LOCKMODES" used in many places.
>
> I don't like this.  Is it possible to write these comparisons in terms
> of what they conflict with?  I think there are two main cases in the
> existing code:
>
> 1. "is this lock mode valid" (sounds reasonable)
> 2. "can this be acquired in hot standby" (not so much, but makes
>sense.)
>
> and now we have your third thing, "what is the strongest of these two
> locks".

The third method already exists in tablecmds.c:
/*
 * Take the greatest lockmode from any subcommand
 */
if (cmd_lockmode > lockmode)
lockmode = cmd_lockmode;

> For instance, if you told me to choose between ShareLock and
> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> don't it's sensible to have the "lock mode compare" primitive honestly.
> I don't have any great ideas to offer ATM sadly.

Yes, the thing is that lowering the lock levels is good for
concurrency, but the non-monotony of the lock levels makes it
impossible to choose an intermediate state correctly. Hence I think
that the one and only safe answer to this problem is that we check if
the locks taken for each subcommand are all the same, and use this
lock. If there is just one lock different, like for example one
subcommand uses ShareLock, and a second one ShareUpdateExclusiveLock
(good example of yours) we simply fallback to AccessExclusiveLock,
based on the fact that we are sure that this lock conflicts with all
the other ones.

At the same time I think that we should as well patch the existing
code path of tablecmds.c that already does those comparisons.
 Regards,
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:

> In this patch I didn't change all lockmode comparison places previous
> pointed by you, but I can change it maybe adding other method called
> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock &&
> lockmode < MAX_LOCKMODES" used in many places.

I don't like this.  Is it possible to write these comparisons in terms
of what they conflict with?  I think there are two main cases in the
existing code:

1. "is this lock mode valid" (sounds reasonable)
2. "can this be acquired in hot standby" (not so much, but makes
   sense.)

and now we have your third thing, "what is the strongest of these two
locks".  For instance, if you told me to choose between ShareLock and
ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
don't it's sensible to have the "lock mode compare" primitive honestly.
I don't have any great ideas to offer ATM sadly.

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


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

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Fri, Jul 31, 2015 at 10:01 AM, Michael Paquier 
wrote:
>
> On Fri, Jul 31, 2015 at 11:41 AM, Fabrízio de Royes Mello wrote:
> >> We usually don't compare lock values that way, i.e. there's not
> >> guaranteed to be a strict monotonicity between lock levels. I don't
> >> really agree with that policy, but it's nonetheless there.
> >
> > And how is the better way to compare lock values to get the highest lock
> > level? Perhaps creating a function to compare lock levels?
>
> I guess that this is exactly what Andres has in mind, aka something
> like LockModeCompare(lockmode, lockmode) that returns {-1,0,1}
> depending on which lock is higher on the hierarchy. This would do
> exactly what your patch is doing though, except that this will
> localize the comparison operators in lock.c. Though I am seeing at
> quick glance a couple of places already do such comparisons:
> backend/commands/tablecmds.c:if (cmd_lockmode > lockmode)
> backend/storage/lmgr/lock.c:lockmode > RowExclusiveLock)
> backend/storage/lmgr/lock.c:if (lockmode >= AccessExclusiveLock &&
> backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
> < MAX_LOCKMODES);
> backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
> < MAX_LOCKMODES);
> backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
> < MAX_LOCKMODES);
> backend/access/index/indexam.c:Assert(lockmode >= NoLock &&
> lockmode < MAX_LOCKMODES);
> All of them are just sanity checks, except the one in tablecmds.c is
> not (2dbbda0). Hence I am thinking that this is not really a problem
> this patch should tackle by itself...
>

I did it in the attached version of the patch... But I don't know if the
names are good so fell free to suggest others if you dislike of my choice.

In this patch I didn't change all lockmode comparison places previous
pointed by you, but I can change it maybe adding other method called
LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock &&
lockmode < MAX_LOCKMODES" used in many places.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..a62ada1 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFAC

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 31, 2015 at 10:05 PM, Simon Riggs  wrote:
> On 31 July 2015 at 02:46, Michael Paquier  wrote:
>
>>
>> > Added. I really don't know if my isolation tests are completely correct,
>> > is
>> > my first time writing this kind of tests.
>>
>> This patch size has increased from 16k to 157k because of the output
>> of the isolation tests you just added.
>
>
> That's too much.

Yes, same opinion as mentioned upthread.

> Why do we need more isolation tests? There isn't anything critical here, its
> just different lock levels for ALTER TABLE. A few normal regression tests
> are fine for this.

Fabrizio went down to 26k with his latest patch by using only a subset
of permutations. To put it shortly, those things are worth testing. We
have the infrastructure to do it, and we lack of coverage in this
area. Hence this patch is a good occasion to do it IMO.
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Simon Riggs]

On 31 July 2015 at 02:46, Michael Paquier  wrote:


> > Added. I really don't know if my isolation tests are completely correct,
> is
> > my first time writing this kind of tests.
>
> This patch size has increased from 16k to 157k because of the output
> of the isolation tests you just added.


That's too much.

Why do we need more isolation tests? There isn't anything critical here,
its just different lock levels for ALTER TABLE. A few normal regression
tests are fine for this.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 31, 2015 at 12:21 PM, Fabrízio de Royes Mello
 wrote:
>
> On Thu, Jul 30, 2015 at 10:46 PM, Michael Paquier wrote:
>
>
>> On further notice, I would recommend not to use the same string name
>> for the session and the query identifiers. And I think that inserting
>> only one tuple at initialization is just but fine.
>>[...]
>> Be careful as well to not include incorrect permutations in the
>> expected output file, the isolation tester is smart enough to ping you
>> about that.
>>
>
> Changed the isolation tests according your suggestions.

Thanks, this looks good, and it reduces the patch size back to 26k. I
am switching that as ready for committer, we could argue more about
the lock levels that could be used for other parameters but ISTM that
this patch is taking the safest approach and we could always revisit
some lock levels afterwards.
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 31, 2015 at 11:41 AM, Fabrízio de Royes Mello wrote:
>> We usually don't compare lock values that way, i.e. there's not
>> guaranteed to be a strict monotonicity between lock levels. I don't
>> really agree with that policy, but it's nonetheless there.
>
> And how is the better way to compare lock values to get the highest lock
> level? Perhaps creating a function to compare lock levels?

I guess that this is exactly what Andres has in mind, aka something
like LockModeCompare(lockmode, lockmode) that returns {-1,0,1}
depending on which lock is higher on the hierarchy. This would do
exactly what your patch is doing though, except that this will
localize the comparison operators in lock.c. Though I am seeing at
quick glance a couple of places already do such comparisons:
backend/commands/tablecmds.c:if (cmd_lockmode > lockmode)
backend/storage/lmgr/lock.c:lockmode > RowExclusiveLock)
backend/storage/lmgr/lock.c:if (lockmode >= AccessExclusiveLock &&
backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/heap/heapam.c:Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/index/indexam.c:Assert(lockmode >= NoLock &&
lockmode < MAX_LOCKMODES);
All of them are just sanity checks, except the one in tablecmds.c is
not (2dbbda0). Hence I am thinking that this is not really a problem
this patch should tackle by itself...
Regards,
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Thu, Jul 30, 2015 at 10:46 PM, Michael Paquier 
wrote:
>
> This patch size has increased from 16k to 157k because of the output
> of the isolation tests you just added. This is definitely too large
> and actually the test coverage is rather limited. Hence I think that
> they should be changed as follows:
> - Use only one table, the locks of tables a and b do not conflict, and
> that's what we want to look at here.
> - Check the locks of all the relation parameters, by that I mean as
> well fillfactor and user_catalog_table which still take
> AccessExclusiveLock on the relation
> - Use custom permutations. I doubt that there is much sense to test
> all the permutations in this context, and this will reduce the
> expected output size drastically.
>

Ok.


> On further notice, I would recommend not to use the same string name
> for the session and the query identifiers. And I think that inserting
> only one tuple at initialization is just but fine.
>
> Here is an example of such a spec:
> setup
> {
>  CREATE TABLE a (id int PRIMARY KEY);
>  INSERT INTO a SELECT generate_series(1,100);
> }
> teardown
> {
>  DROP TABLE a;
> }
> session "s1"
> step "b1"   { BEGIN; }
> # TODO add one query per parameter
> step "at11" { ALTER TABLE a SET (fillfactor=10); }
> step "at12" { ALTER TABLE a SET
(autovacuum_vacuum_scale_factor=0.001); }
> step "c1"   { COMMIT; }
> session "s2"
> step "b2"   { BEGIN; }
> step "wx1"  { UPDATE a SET id = id + 1; }
> step "c2"   { COMMIT; }
>
> And by testing permutations like for example that it is possible to
> see which session is waiting for what. Input:
> permutation "b1" "b2" "at11" "wx1" "c1" "c2"
> Output where session 2 waits for lock taken after fillfactor update:
> step b1: BEGIN;
> step b2: BEGIN;
> step at11: ALTER TABLE a SET (fillfactor=10);
> step wx1: UPDATE a SET id = id + 1; 
> step c1: COMMIT;
> step wx1: <... completed>
> step c2: COMMIT;
>
> Be careful as well to not include incorrect permutations in the
> expected output file, the isolation tester is smart enough to ping you
> about that.
>

Changed the isolation tests according your suggestions.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..b8d2a92 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 31, 2015 at 11:28 AM, Andres Freund  wrote:
>
>> @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
>
> If we go through this list, I'd rather make informed decisions about
> each reloption. Otherwise we're going to get patches for each of them
> separately over the next versions.

Just dropping quickly a reply: I meant table relopts only, excluding
the index stuff for now regarding the isolation tests.

>> + AccessExclusiveLock
>> + foreach(cell, defList)
>> + {
>> + DefElem *def = (DefElem *) lfirst(cell);
>> + int i;
>> +
>> + for (i = 0; relOpts[i]; i++)
>> + {
>> + if (pg_strncasecmp(relOpts[i]->name, def->defname, 
>> relOpts[i]->namelen + 1) == 0)
>> + {
>> + if (lockmode < relOpts[i]->lockmode)
>> + lockmode = relOpts[i]->lockmode;
>> + }
>> + }
>> + }
>> +
>> + return lockmode;
>> +}
>
> We usually don't compare lock values that way, i.e. there's not
> guaranteed to be a strict monotonicity between lock levels. I don't
> really agree with that policy, but it's nonetheless there.

Yeah, there are some in lock.c but that's rather localized.
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Thu, Jul 30, 2015 at 11:28 PM, Andres Freund  wrote:
>
> > @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
>
> If we go through this list, I'd rather make informed decisions about
> each reloption. Otherwise we're going to get patches for each of them
> separately over the next versions.
>

I have no problem to do this change now instead of wait for next versions...


> > @@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
> >   {
> >   "fastupdate",
> >   "Enables \"fast update\" feature for this GIN
index",
> > - RELOPT_KIND_GIN
> > + RELOPT_KIND_GIN,
> > + AccessExclusiveLock
> >   },
> >   true
> >   },
>
>
> > @@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
> >   {
> >   "fillfactor",
> >   "Packs table pages only to this percentage",
> > - RELOPT_KIND_HEAP
> > + RELOPT_KIND_HEAP,
> > + AccessExclusiveLock
> >   },
> >   HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
>
> > [some other fillfactor settings]
>
> >   {
> >   {
> >   "gin_pending_list_limit",
> >   "Maximum size of the pending list for this GIN
index, in kilobytes.",
> > - RELOPT_KIND_GIN
> > + RELOPT_KIND_GIN,
> > + AccessExclusiveLock
> >   },
> >   -1, 64, MAX_KILOBYTES
> >   },
> > @@ -297,7 +325,8 @@ static relopt_string stringRelOpts[] =
> >   {
> >   "buffering",
> >   "Enables buffering build for this GiST index",
> > - RELOPT_KIND_GIST
> > + RELOPT_KIND_GIST,
> > + AccessExclusiveLock
> >   },
> >   4,
> >   false,
>
> Why? These options just change things for the future and don't influence
> past decisions. It seems unproblematic to change them.
>

+1


> > @@ -259,7 +283,8 @@ static relopt_real realRelOpts[] =
> >   {
> >   "seq_page_cost",
> >   "Sets the planner's estimate of the cost of a
sequentially fetched disk page.",
> > - RELOPT_KIND_TABLESPACE
> > + RELOPT_KIND_TABLESPACE,
> > + AccessExclusiveLock
> >   },
> >   -1, 0.0, DBL_MAX
> >   },
> > @@ -267,7 +292,8 @@ static relopt_real realRelOpts[] =
> >   {
> >   "random_page_cost",
> >   "Sets the planner's estimate of the cost of a
nonsequentially fetched disk page.",
> > - RELOPT_KIND_TABLESPACE
> > + RELOPT_KIND_TABLESPACE,
> > + AccessExclusiveLock
> >   },
> >   -1, 0.0, DBL_MAX
> >   },
> > @@ -275,7 +301,8 @@ static relopt_real realRelOpts[] =
> >   {
> >   "n_distinct",
> >   "Sets the planner's estimate of the number of
distinct values appearing in a column (excluding child relations).",
> > - RELOPT_KIND_ATTRIBUTE
> > + RELOPT_KIND_ATTRIBUTE,
> > + AccessExclusiveLock
> >   },
> >   0, -1.0, DBL_MAX
> >   },
> > @@ -283,7 +310,8 @@ static relopt_real realRelOpts[] =
> >   {
> >   "n_distinct_inherited",
> >   "Sets the planner's estimate of the number of
distinct values appearing in a column (including child relations).",
> > - RELOPT_KIND_ATTRIBUTE
> > + RELOPT_KIND_ATTRIBUTE,
> > + AccessExclusiveLock
> >   },
> >   0, -1.0, DBL_MAX
> >   },
>
> These probably are the settings that are most interesting to change
> without access exlusive locks.
>

+1


> >   j = 0;
> >   for (i = 0; boolRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
> > +
 boolRelOpts[i].gen.lockmode));
> >   j++;
> > + }
> >   for (i = 0; intRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
> > +
 intRelOpts[i].gen.lockmode));
> >   j++;
> > + }
> >   for (i = 0; realRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
> > +
 realRelOpts[i].gen.lockmode));
> >   j++;
> > + }
> >   for (i = 0; stringRelOpts[i].gen.name; i++)
> > + {
> > + Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
> > +
 stringRelOpts[i].gen.lockmode));
> >   j++;
> > + }
> >   j += num_custom_options;
>
> 

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Andres Freund]

> @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =

If we go through this list, I'd rather make informed decisions about
each reloption. Otherwise we're going to get patches for each of them
separately over the next versions.

> @@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
>   {
>   "fastupdate",
>   "Enables \"fast update\" feature for this GIN index",
> - RELOPT_KIND_GIN
> + RELOPT_KIND_GIN,
> + AccessExclusiveLock
>   },
>   true
>   },


> @@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
>   {
>   "fillfactor",
>   "Packs table pages only to this percentage",
> - RELOPT_KIND_HEAP
> + RELOPT_KIND_HEAP,
> + AccessExclusiveLock
>   },
>   HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100

> [some other fillfactor settings]

>   {
>   {
>   "gin_pending_list_limit",
>   "Maximum size of the pending list for this GIN index, 
> in kilobytes.",
> - RELOPT_KIND_GIN
> + RELOPT_KIND_GIN,
> + AccessExclusiveLock
>   },
>   -1, 64, MAX_KILOBYTES
>   },
> @@ -297,7 +325,8 @@ static relopt_string stringRelOpts[] =
>   {
>   "buffering",
>   "Enables buffering build for this GiST index",
> - RELOPT_KIND_GIST
> + RELOPT_KIND_GIST,
> + AccessExclusiveLock
>   },
>   4,
>   false,

Why? These options just change things for the future and don't influence
past decisions. It seems unproblematic to change them.

> @@ -259,7 +283,8 @@ static relopt_real realRelOpts[] =
>   {
>   "seq_page_cost",
>   "Sets the planner's estimate of the cost of a 
> sequentially fetched disk page.",
> - RELOPT_KIND_TABLESPACE
> + RELOPT_KIND_TABLESPACE,
> + AccessExclusiveLock
>   },
>   -1, 0.0, DBL_MAX
>   },
> @@ -267,7 +292,8 @@ static relopt_real realRelOpts[] =
>   {
>   "random_page_cost",
>   "Sets the planner's estimate of the cost of a 
> nonsequentially fetched disk page.",
> - RELOPT_KIND_TABLESPACE
> + RELOPT_KIND_TABLESPACE,
> + AccessExclusiveLock
>   },
>   -1, 0.0, DBL_MAX
>   },
> @@ -275,7 +301,8 @@ static relopt_real realRelOpts[] =
>   {
>   "n_distinct",
>   "Sets the planner's estimate of the number of distinct 
> values appearing in a column (excluding child relations).",
> - RELOPT_KIND_ATTRIBUTE
> + RELOPT_KIND_ATTRIBUTE,
> + AccessExclusiveLock
>   },
>   0, -1.0, DBL_MAX
>   },
> @@ -283,7 +310,8 @@ static relopt_real realRelOpts[] =
>   {
>   "n_distinct_inherited",
>   "Sets the planner's estimate of the number of distinct 
> values appearing in a column (including child relations).",
> - RELOPT_KIND_ATTRIBUTE
> + RELOPT_KIND_ATTRIBUTE,
> + AccessExclusiveLock
>   },
>   0, -1.0, DBL_MAX
>   },

These probably are the settings that are most interesting to change
without access exlusive locks.

>   j = 0;
>   for (i = 0; boolRelOpts[i].gen.name; i++)
> + {
> + Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
> +
> boolRelOpts[i].gen.lockmode));
>   j++;
> + }
>   for (i = 0; intRelOpts[i].gen.name; i++)
> + {
> + Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
> +
> intRelOpts[i].gen.lockmode));
>   j++;
> + }
>   for (i = 0; realRelOpts[i].gen.name; i++)
> + {
> + Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
> +
> realRelOpts[i].gen.lockmode));
>   j++;
> + }
>   for (i = 0; stringRelOpts[i].gen.name; i++)
> + {
> + Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
> +
> stringRelOpts[i].gen.lockmode));
>   j++;
> + }
>   j += num_custom_options;

Doesn't really seem worth it to assert individually in each case here to
me.

> +/*
> + * Determine the required LO

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 31, 2015 at 8:30 AM, Fabrízio de Royes Mello
 wrote:
>
> On Fri, Jul 24, 2015 at 4:05 AM, Michael Paquier 
> wrote:
>>
>> On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
>>  wrote:
>> > On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs 
>> > wrote:
>> >> Looks functionally complete
>> >>
>> >> Need a test to show that ALTER TABLE works on views, as discussed on
>> >> this
>> >> thread. And confirmation that pg_dump is not broken by this.
>> >>
>> >> Message-ID: 20140321205828.gb3969...@tornado.leadboat.com
>> >>
>> >
>> > Added more test cases to cover ALTER TABLE on views.
>> >
>> > I'm thinking about the isolation tests, what about add another
>> > 'alter-table'
>> > spec for isolation tests enabling and disabling 'autovacuum' options?
>>
>> Yes, please.
>>
>
> Added. I really don't know if my isolation tests are completely correct, is
> my first time writing this kind of tests.

This patch size has increased from 16k to 157k because of the output
of the isolation tests you just added. This is definitely too large
and actually the test coverage is rather limited. Hence I think that
they should be changed as follows:
- Use only one table, the locks of tables a and b do not conflict, and
that's what we want to look at here.
- Check the locks of all the relation parameters, by that I mean as
well fillfactor and user_catalog_table which still take
AccessExclusiveLock on the relation
- Use custom permutations. I doubt that there is much sense to test
all the permutations in this context, and this will reduce the
expected output size drastically.

On further notice, I would recommend not to use the same string name
for the session and the query identifiers. And I think that inserting
only one tuple at initialization is just but fine.

Here is an example of such a spec:
setup
{
 CREATE TABLE a (id int PRIMARY KEY);
 INSERT INTO a SELECT generate_series(1,100);
}
teardown
{
 DROP TABLE a;
}
session "s1"
step "b1"   { BEGIN; }
# TODO add one query per parameter
step "at11" { ALTER TABLE a SET (fillfactor=10); }
step "at12" { ALTER TABLE a SET (autovacuum_vacuum_scale_factor=0.001); }
step "c1"   { COMMIT; }
session "s2"
step "b2"   { BEGIN; }
step "wx1"  { UPDATE a SET id = id + 1; }
step "c2"   { COMMIT; }

And by testing permutations like for example that it is possible to
see which session is waiting for what. Input:
permutation "b1" "b2" "at11" "wx1" "c1" "c2"
Output where session 2 waits for lock taken after fillfactor update:
step b1: BEGIN;
step b2: BEGIN;
step at11: ALTER TABLE a SET (fillfactor=10);
step wx1: UPDATE a SET id = id + 1; 
step c1: COMMIT;
step wx1: <... completed>
step c2: COMMIT;

Be careful as well to not include incorrect permutations in the
expected output file, the isolation tester is smart enough to ping you
about that.

>> +GetRelOptionsLockLevel(List *defList)
>> +{
>> +   LOCKMODElockmode = NoLock;
>> Shouldn't this default to AccessExclusiveLock instead of NoLock?
>>
>
> No, because it will break the logic bellow (get the highest locklevel
> according the defList), but I force return an AccessExclusiveLock if
> "defList == NIL".

Yep, OK with this change.

The rest of the patch looks good to me, so once the isolation test
coverage is done I think that it could be put in the hands of a
committer.
Regards
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
 wrote:
> On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs  wrote:
>> Looks functionally complete
>>
>> Need a test to show that ALTER TABLE works on views, as discussed on this
>> thread. And confirmation that pg_dump is not broken by this.
>>
>> Message-ID: 20140321205828.gb3969...@tornado.leadboat.com
>>
>
> Added more test cases to cover ALTER TABLE on views.
>
> I'm thinking about the isolation tests, what about add another 'alter-table'
> spec for isolation tests enabling and disabling 'autovacuum' options?

Yes, please.

> I did some tests using ALTER TABLE on views and also ALTER VIEW and I didn't
> identify any anomalies.
>
>> Needs documentation
>>
>
> Added.

for (i = 0; boolRelOpts[i].gen.name; i++)
+   {
+
Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
boolRelOpts[i].gen.lockmode));
j++;
+   }
for (i = 0; intRelOpts[i].gen.name; i++)
+   {
+   Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
intRelOpts[i].gen.lockmode));
j++;
+   }
for (i = 0; realRelOpts[i].gen.name; i++)
+   {
+
Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
realRelOpts[i].gen.lockmode));
j++;
+   }
for (i = 0; stringRelOpts[i].gen.name; i++)
+   {
+
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
stringRelOpts[i].gen.lockmode));
j++;
+   }
Splitting those long lines into two will avoid some work for pgindent.

+GetRelOptionsLockLevel(List *defList)
+{
+   LOCKMODElockmode = NoLock;
Shouldn't this default to AccessExclusiveLock instead of NoLock?
-- 
Michael


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs  wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
>
> Looks functionally complete
>
> Need a test to show that ALTER TABLE works on views, as discussed on this
thread. And confirmation that pg_dump is not broken by this.
>
> Message-ID: 20140321205828.gb3969...@tornado.leadboat.com
>

Added more test cases to cover ALTER TABLE on views.

I'm thinking about the isolation tests, what about add another
'alter-table' spec for isolation tests enabling and disabling 'autovacuum'
options?

I did some tests using ALTER TABLE on views and also ALTER VIEW and I
didn't identify any anomalies.


> Needs documentation
>

Added.

Att,


*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 207fec1..b126855 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -541,6 +541,10 @@ ALTER TABLE ALL IN TABLESPACE name
   of ALTER TABLE that forces a table rewrite.
  
 
+ 
+  Changing autovacuum storage parameters acquires a SHARE UPDATE EXCLUSIVE lock.
+ 
+
  
   
While CREATE TABLE allows OIDS to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f273944 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_analyze_threshold",
 			"Minimum number of tuple inserts, updates or deletes prior to analyze",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_cost_delay",
 			"Vacuum cost delay in milliseconds, for autovacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Simon Riggs]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Looks functionally complete

Need a test to show that ALTER TABLE works on views, as discussed on this 
thread. And confirmation that pg_dump is not broken by this.

Message-ID: 20140321205828.gb3969...@tornado.leadboat.com

Needs documentation

The new status of this patch is: Waiting on Author


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Tue, Apr 7, 2015 at 10:57 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera 
wrote:
> >
> > Fabrízio de Royes Mello wrote:
> > > On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <
alvhe...@2ndquadrant.com>
> > > wrote:
> > > >
> > > > Fabrízio de Royes Mello wrote:
> > > >
> > > > > Ok guys. The attached patch refactor the reloptions adding a new
field
> > > > > "lockmode" in "relopt_gen" struct and a new method to determine
the
> > > > > required lock level from an option list.
> > > > >
> > > > > We need determine the appropriate lock level for each reloption:
> > > >
> > > > I don't think AccessShareLock is appropriate for any option
change.  You
> > > > should be using a lock level that's self-conflicting, as a minimum
> > > > requirement, to avoid two processes changing the value concurrently.
> > >
> > > What locklevel do you suggest? Maybe ShareLock?
> >
> > ShareUpdateExclusive probably.  ShareLock doesn't conflict with itself.
> >
>
> Ok.
>
>
> > > > (I would probably go as far as ensuring that the lock level
specified in
> > > > the table DoLockModesConflict() with itself in an Assert somewhere.)
> > >
> > > If I understood this is to check if the locklevel of the reloption
list
> > > don't conflict one each other, is it?
> >
> > I mean
> > Assert(DoLockModesConflict(relopt_gen->locklevel,
relopt_gen->locklevel));
> >
>
> Understood... IMHO the better place to perform this assert is in
"initialize_reloptions".
>
> Thoughts?
>

The attached patch:
- introduce new field "lockmode" to "relopt_gen" struct
- initialize lockmode with ShareUpdateExclusiveLock to autovac settings
- add assert to ensuring that the lock level specified don't conflict with
itself
- add function GetRelOptionsLockLevel to determine the required lock mode
from an option list

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f273944 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOPT_KIND_GIST,
+			AccessExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
-			RELOPT_KIND_SPGIST
+			RELOPT_KIND_SPGIST,
+			AccessExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
 		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			ShareUpdateExclusiveLock
 		},
 		-1, 0, INT_MAX
 	},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
 		{
 			"au

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera 
wrote:
>
> Fabrízio de Royes Mello wrote:
> > On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <
alvhe...@2ndquadrant.com>
> > wrote:
> > >
> > > Fabrízio de Royes Mello wrote:
> > >
> > > > Ok guys. The attached patch refactor the reloptions adding a new
field
> > > > "lockmode" in "relopt_gen" struct and a new method to determine the
> > > > required lock level from an option list.
> > > >
> > > > We need determine the appropriate lock level for each reloption:
> > >
> > > I don't think AccessShareLock is appropriate for any option change.
You
> > > should be using a lock level that's self-conflicting, as a minimum
> > > requirement, to avoid two processes changing the value concurrently.
> >
> > What locklevel do you suggest? Maybe ShareLock?
>
> ShareUpdateExclusive probably.  ShareLock doesn't conflict with itself.
>

Ok.


> > > (I would probably go as far as ensuring that the lock level specified
in
> > > the table DoLockModesConflict() with itself in an Assert somewhere.)
> >
> > If I understood this is to check if the locklevel of the reloption list
> > don't conflict one each other, is it?
>
> I mean
> Assert(DoLockModesConflict(relopt_gen->locklevel,
relopt_gen->locklevel));
>

Understood... IMHO the better place to perform this assert is in
"initialize_reloptions".

Thoughts?

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:
> On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera 
> wrote:
> >
> > Fabrízio de Royes Mello wrote:
> >
> > > Ok guys. The attached patch refactor the reloptions adding a new field
> > > "lockmode" in "relopt_gen" struct and a new method to determine the
> > > required lock level from an option list.
> > >
> > > We need determine the appropriate lock level for each reloption:
> >
> > I don't think AccessShareLock is appropriate for any option change.  You
> > should be using a lock level that's self-conflicting, as a minimum
> > requirement, to avoid two processes changing the value concurrently.
> 
> What locklevel do you suggest? Maybe ShareLock?

ShareUpdateExclusive probably.  ShareLock doesn't conflict with itself.

> > (I would probably go as far as ensuring that the lock level specified in
> > the table DoLockModesConflict() with itself in an Assert somewhere.)
> 
> If I understood this is to check if the locklevel of the reloption list
> don't conflict one each other, is it?

I mean
Assert(DoLockModesConflict(relopt_gen->locklevel, 
relopt_gen->locklevel));

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


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

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera 
wrote:
>
> Fabrízio de Royes Mello wrote:
>
> > Ok guys. The attached patch refactor the reloptions adding a new field
> > "lockmode" in "relopt_gen" struct and a new method to determine the
> > required lock level from an option list.
> >
> > We need determine the appropriate lock level for each reloption:
>
> I don't think AccessShareLock is appropriate for any option change.  You
> should be using a lock level that's self-conflicting, as a minimum
> requirement, to avoid two processes changing the value concurrently.

What locklevel do you suggest? Maybe ShareLock?


> (I would probably go as far as ensuring that the lock level specified in
> the table DoLockModesConflict() with itself in an Assert somewhere.)
>

If I understood this is to check if the locklevel of the reloption list
don't conflict one each other, is it?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:

> Ok guys. The attached patch refactor the reloptions adding a new field
> "lockmode" in "relopt_gen" struct and a new method to determine the
> required lock level from an option list.
> 
> We need determine the appropriate lock level for each reloption:

I don't think AccessShareLock is appropriate for any option change.  You
should be using a lock level that's self-conflicting, as a minimum
requirement, to avoid two processes changing the value concurrently.  (I
would probably go as far as ensuring that the lock level specified in
the table DoLockModesConflict() with itself in an Assert somewhere.)

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


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

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Wed, Apr 1, 2015 at 1:45 AM, Noah Misch  wrote:
>
> On Tue, Mar 31, 2015 at 01:17:03PM -0400, Robert Haas wrote:
> > On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
> >  wrote:
> > > Attached a very WIP patch to reduce lock level when setting autovacuum
> > > reloptions in "ALTER TABLE .. SET ( .. )" statement.
> >
> > I think the first thing we need to here is analyze all of the options
> > and determine what the appropriate lock level is for each, and why.
>
> Agreed.  Fabrízio, see this message for the discussion that led to the
code
> comment you found (search for "relopt_gen"):
>
>
http://www.postgresql.org/message-id/20140321034556.ga3927...@tornado.leadboat.com

Ok guys. The attached patch refactor the reloptions adding a new field
"lockmode" in "relopt_gen" struct and a new method to determine the
required lock level from an option list.

We need determine the appropriate lock level for each reloption:

- boolRelopts:
  * autovacuum_enabled  (AccessShareLock)
  * user_catalog_table  (AccessExclusiveLock)
  * fastupdate  (AccessExclusiveLock)
  * security_barrier  (AccessExclusiveLock)

- intRelOpts:
  * fillfactor (heap)  (AccessExclusiveLock)
  * fillfactor (btree)  (AccessExclusiveLock)
  * fillfactor (gist)  (AccessExclusiveLock)
  * fillfactor (spgist)  (AccessExclusiveLock)
  * autovacuum_vacuum_threshold  (AccessShareLock)
  * autovacuum_analyze_threshold  (AccessShareLock)
  * autovacuum_vacuum_cost_delay  (AccessShareLock)
  * autovacuum_vacuum_cost_limit  (AccessShareLock)
  * autovacuum_freeze_min_age  (AccessShareLock)
  * autovacuum_multixact_freeze_min_age  (AccessShareLock)
  * autovacuum_freeze_max_age  (AccessShareLock)
  * autovacuum_multixact_freeze_max_age  (AccessShareLock)
  * autovacuum_freeze_table_age  (AccessShareLock)
  * autovacuum_multixact_freeze_table_age  (AccessShareLock)
  * log_autovacuum_min_duration  (AccessShareLock)
  * pages_per_range  (AccessExclusiveLock)
  * gin_pending_list_limit  (AccessExclusiveLock)

- realRelOpts:
  * autovacuum_vacuum_scale_factor  (AccessShareLock)
  * autovacuum_analyze_scale_factor  (AccessShareLock)
  * seq_page_cost  (AccessExclusiveLock)
  * random_page_cost  (AccessExclusiveLock)
  * n_distinct  (AccessExclusiveLock)
  * n_distinct_inherited  (AccessExclusiveLock)

- stringRelOpts:
  * buffering  (AccessExclusiveLock)
  * check_option  (AccessExclusiveLock)


In the above list I just change lock level from AccessExclusiveLock to
AccessShareLock to all "autovacuum" related reloptions because it was the
motivation of this patch.

I need some help to define the others.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f83ce2f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"autovacuum_enabled",
 			"Enables autovacuum in this relation",
-			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"user_catalog_table",
 			"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
-			RELOPT_KIND_GIN
+			RELOPT_KIND_GIN,
+			AccessExclusiveLock
 		},
 		true
 	},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
 		{
 			"security_barrier",
 			"View acts as a row security barrier",
-			RELOPT_KIND_VIEW
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
 		},
 		false
 	},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs table pages only to this percentage",
-			RELOPT_KIND_HEAP
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
-			RELOPT_KIND_BTREE
+			RELOPT_KIND_BTREE,
+			AccessExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
-			RELOPT_KIND_HASH
+			RELOPT_KIND_HASH,
+			AccessExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
 		{
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
-			RELOPT_KIND_GIST
+			RELOP

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Noah Misch]

On Tue, Mar 31, 2015 at 01:17:03PM -0400, Robert Haas wrote:
> On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
>  wrote:
> > Attached a very WIP patch to reduce lock level when setting autovacuum
> > reloptions in "ALTER TABLE .. SET ( .. )" statement.
> 
> I think the first thing we need to here is analyze all of the options
> and determine what the appropriate lock level is for each, and why.

Agreed.  Fabrízio, see this message for the discussion that led to the code
comment you found (search for "relopt_gen"):

  
http://www.postgresql.org/message-id/20140321034556.ga3927...@tornado.leadboat.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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Robert Haas]

On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
 wrote:
> Attached a very WIP patch to reduce lock level when setting autovacuum
> reloptions in "ALTER TABLE .. SET ( .. )" statement.

I think the first thing we need to here is analyze all of the options
and determine what the appropriate lock level is for each, and why.

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


-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Mon, Mar 30, 2015 at 8:14 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
>
> On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby 
wrote:
> >
> > On 3/27/15 2:23 PM, Fabrízio de Royes Mello wrote:
> >>
> >> Hi all,
> >>
> >> I'm tweaking some autovacuum settings in a table with high write usage
> >> but with ALTER TABLE .. SET ( .. ) this task was impossible, so I did a
> >> catalog update  (pg_class) to change reloptions.
> >>
> >> Maybe it's a stupid doubt, but why we need to get an
AccessExclusiveLock
> >> on relation to set reloptions if we just touch in pg_class tuples
> >> (RowExclusiveLock) ?
> >
> >
> > For a very long time catalog access was not MVCC safe. I think that's
been changed, so at this point it may be OK to relax the lock, at least in
the case of autovac settings. There may well be other settings in there
where it would not be safe.
> >
>
> Hummm There are a comment in AlterTableGetLockLevel:
>
>  3017 /*
>  3018  * Rel options are more complex than first appears.
Options
>  3019  * are set here for tables, views and indexes; for
historical
>  3020  * reasons these can all be used with ALTER TABLE,
so we can't
>  3021  * decide between them using the basic grammar.
>  3022  *
>  3023  * XXX Look in detail at each option to determine
lock level,
>  3024  * e.g. cmd_lockmode = GetRelOptionsLockLevel((List
*)
>  3025  * cmd->def);
>  3026  */
>  3027 case AT_SetRelOptions:  /* Uses MVCC in
getIndexes() and
>  3028  * getTables() */
>  3029 case AT_ResetRelOptions:/* Uses MVCC in
getIndexes() and
>  3030  * getTables() */
>  3031 cmd_lockmode = AccessExclusiveLock;
>  3032 break;
>
>
> Maybe it's time to implement "GetRelOptionsLockLevel" to relax the lock
to autovac settings (AccessShareLock). To other settings we continue using
AccessExclusiveLock.
>
> There are some objection to implement in that way?
>

Attached a very WIP patch to reduce lock level when setting autovacuum
reloptions in "ALTER TABLE .. SET ( .. )" statement.

I confess the implementation is ugly, maybe we should add a new item to
reloptions constants in src/backend/access/common/reloptions.c and a proper
function to get lock level by reloption. Thoughts?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 32e19c5..0be658f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -402,6 +402,7 @@ static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmo
 static void ATExecSetRelOptions(Relation rel, List *defList,
 	AlterTableType operation,
 	LOCKMODE lockmode);
+static LOCKMODE GetRelOptionsLockLevel(List *defList);
 static void ATExecEnableDisableTrigger(Relation rel, char *trigname,
 	   char fires_when, bool skip_system, LOCKMODE lockmode);
 static void ATExecEnableDisableRule(Relation rel, char *rulename,
@@ -2783,6 +2784,39 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse)
 }
 
 /*
+ * GetRelOptionsLockLevel
+ *
+ * Return AccessShareLock if all reloptions is related to autovacuum
+ * else return AccessExclusiveLock.
+ *
+ */
+LOCKMODE
+GetRelOptionsLockLevel(List *defList)
+{
+	LOCKMODE	lockmode;
+	ListCell	*cell;
+
+	if (defList == NIL)
+		return NoLock;
+
+	foreach(cell, defList)
+	{
+		DefElem	*def = (DefElem *) lfirst(cell);
+
+		/* relax lock for autovacuum reloptions */
+		if (pg_strncasecmp("autovacuum_", def->defname, 11) == 0)
+			lockmode = AccessShareLock;
+		else
+		{
+			lockmode = AccessExclusiveLock;
+			break;
+		}
+	}
+
+	return lockmode;
+}
+
+/*
  * AlterTableGetLockLevel
  *
  * Sets the overall lock level required for the supplied list of subcommands.
@@ -3028,7 +3062,7 @@ AlterTableGetLockLevel(List *cmds)
 		 * getTables() */
 			case AT_ResetRelOptions:	/* Uses MVCC in getIndexes() and
 		 * getTables() */
-cmd_lockmode = AccessExclusiveLock;
+cmd_lockmode = GetRelOptionsLockLevel((List *) cmd->def);
 break;
 
 			default:			/* oops */

-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby  wrote:
>
> On 3/27/15 2:23 PM, Fabrízio de Royes Mello wrote:
>>
>> Hi all,
>>
>> I'm tweaking some autovacuum settings in a table with high write usage
>> but with ALTER TABLE .. SET ( .. ) this task was impossible, so I did a
>> catalog update  (pg_class) to change reloptions.
>>
>> Maybe it's a stupid doubt, but why we need to get an AccessExclusiveLock
>> on relation to set reloptions if we just touch in pg_class tuples
>> (RowExclusiveLock) ?
>
>
> For a very long time catalog access was not MVCC safe. I think that's
been changed, so at this point it may be OK to relax the lock, at least in
the case of autovac settings. There may well be other settings in there
where it would not be safe.
>

Hummm There are a comment in AlterTableGetLockLevel:

 3017 /*
 3018  * Rel options are more complex than first appears.
Options
 3019  * are set here for tables, views and indexes; for
historical
 3020  * reasons these can all be used with ALTER TABLE, so
we can't
 3021  * decide between them using the basic grammar.
 3022  *
 3023  * XXX Look in detail at each option to determine
lock level,
 3024  * e.g. cmd_lockmode = GetRelOptionsLockLevel((List *)
 3025  * cmd->def);
 3026  */
 3027 case AT_SetRelOptions:  /* Uses MVCC in getIndexes()
and
 3028  * getTables() */
 3029 case AT_ResetRelOptions:/* Uses MVCC in getIndexes()
and
 3030  * getTables() */
 3031 cmd_lockmode = AccessExclusiveLock;
 3032 break;


Maybe it's time to implement "GetRelOptionsLockLevel" to relax the lock to
autovac settings (AccessShareLock). To other settings we continue using
AccessExclusiveLock.

There are some objection to implement in that way?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Jim Nasby]

On 3/27/15 2:23 PM, Fabrízio de Royes Mello wrote:

Hi all,

I'm tweaking some autovacuum settings in a table with high write usage
but with ALTER TABLE .. SET ( .. ) this task was impossible, so I did a
catalog update  (pg_class) to change reloptions.

Maybe it's a stupid doubt, but why we need to get an AccessExclusiveLock
on relation to set reloptions if we just touch in pg_class tuples
(RowExclusiveLock) ?


For a very long time catalog access was not MVCC safe. I think that's 
been changed, so at this point it may be OK to relax the lock, at least 
in the case of autovac settings. There may well be other settings in 
there where it would not be safe.

--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

[HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Fabrízio de Royes Mello]

Hi all,

I'm tweaking some autovacuum settings in a table with high write usage but
with ALTER TABLE .. SET ( .. ) this task was impossible, so I did a catalog
update  (pg_class) to change reloptions.

Maybe it's a stupid doubt, but why we need to get an AccessExclusiveLock on
relation to set reloptions if we just touch in pg_class tuples
(RowExclusiveLock) ?

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

[HACKERS] Doubt Regarding changes to disable keepalives in walsender

[Amit Kapila]

In commit da4efa13d801ccc179f1d2c24d8a60c4a2f8ede9, sending of keepalive
messages have been disabled in walsender. 
Commit message is " Turn off WalSender keepalives by default, users can
enable if desired."

But I am not able to see how users can enable it, can you please point me if
I am missing something?

 

This doubt came as I am implementing replication timeout for walreceiver and
for that walsender has to send some form of heartbeat message at predefined
interval. I wanted to use keepalive to achieve the same, but then noticed
the above and got stuck.

 

 

With Regards,

Amit Kapila.

Re: [HACKERS] Doubt about boundParams

[Ashutosh Bapat]

On Tue, Aug 2, 2011 at 3:45 PM, Heikki Linnakangas <
heikki.linnakan...@enterprisedb.com> wrote:

> On 02.08.2011 12:54, Ashutosh Bapat wrote:
>
>> Hi All,
>> I am looking at usage of bound parameters.
>>
>> In functions SPI_cursor_open_with_args() and SPI_cursor_open_with_args()
>> parameters are flagged as constants and passed to the planner in following
>> manner,
>> paramLI = _SPI_convert_params(nargs, argtypes,
>>Values, Nulls,
>>PARAM_FLAG_CONST);
>>
>>  _SPI_prepare_plan(src,&plan, paramLI);
>>
>> The bound params "paramLI" are then passed to the planner as boundParams.
>> Before actually planning the query, these parameters are evaluated duing
>> constant evaluation (eval_const_expressions_**mutator()), and the Param
>> nodes
>> are replaced with Constant nodes.
>> Further, while executing such queries we pass the paramLI structure to the
>> execution routine e.g. _SPI_execute_plan(). These parameter values are
>> stored in "EState" structure. But, since these parameters are already
>> folded
>> into queries, it looks like parameter values stored in EState are never
>> used.
>>
>> Is this correct? Or somewhere we use those parameter values?
>>
>
> That is correct, at the moment. PARAM_FLAG_CONST means that the planner is
> free evaluate the params during planning, but it doesn't have to. You still
> need to pass the params in _SPI_execute_plan() in case the planner decided
> to not convert some params to Consts, even though as the code stands today
> it always will.
>

Ok, thanks.


>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com
>



-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company

Re: [HACKERS] Doubt about boundParams

[Heikki Linnakangas]

On 02.08.2011 12:54, Ashutosh Bapat wrote:

Hi All,
I am looking at usage of bound parameters.

In functions SPI_cursor_open_with_args() and SPI_cursor_open_with_args()
parameters are flagged as constants and passed to the planner in following
manner,
paramLI = _SPI_convert_params(nargs, argtypes,
Values, Nulls,
PARAM_FLAG_CONST);

  _SPI_prepare_plan(src,&plan, paramLI);

The bound params "paramLI" are then passed to the planner as boundParams.
Before actually planning the query, these parameters are evaluated duing
constant evaluation (eval_const_expressions_mutator()), and the Param nodes
are replaced with Constant nodes.
Further, while executing such queries we pass the paramLI structure to the
execution routine e.g. _SPI_execute_plan(). These parameter values are
stored in "EState" structure. But, since these parameters are already folded
into queries, it looks like parameter values stored in EState are never
used.

Is this correct? Or somewhere we use those parameter values?


That is correct, at the moment. PARAM_FLAG_CONST means that the planner 
is free evaluate the params during planning, but it doesn't have to. You 
still need to pass the params in _SPI_execute_plan() in case the planner 
decided to not convert some params to Consts, even though as the code 
stands today it always will.


--
  Heikki Linnakangas
  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

[HACKERS] Doubt about boundParams

[Ashutosh Bapat]

Hi All,
I am looking at usage of bound parameters.

In functions SPI_cursor_open_with_args() and SPI_cursor_open_with_args()
parameters are flagged as constants and passed to the planner in following
manner,
paramLI = _SPI_convert_params(nargs, argtypes,
   Values, Nulls,
   PARAM_FLAG_CONST);

 _SPI_prepare_plan(src, &plan, paramLI);

The bound params "paramLI" are then passed to the planner as boundParams.
Before actually planning the query, these parameters are evaluated duing
constant evaluation (eval_const_expressions_mutator()), and the Param nodes
are replaced with Constant nodes.
Further, while executing such queries we pass the paramLI structure to the
execution routine e.g. _SPI_execute_plan(). These parameter values are
stored in "EState" structure. But, since these parameters are already folded
into queries, it looks like parameter values stored in EState are never
used.

Is this correct? Or somewhere we use those parameter values?


-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company

Re: [HACKERS] Doubt

[Robert Haas]

On May 25, 2009, at 9:41 AM, jibin jose  wrote:


which tool is used as profiler in postgresql


There is some code in the backend to support gprof; I know that some  
people have had good luck with oprofile, too, which doesn't require  
any special support.


...Robert

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

[HACKERS] Doubt

[jibin jose]

which tool is used as profiler in postgresql

Re: [HACKERS] Doubt in index subplan query

[Decibel!]

On Jun 20, 2008, at 1:11 AM, Suresh wrote:

I have a query plan for a certain query

 Nested Loop  (cost=1.00..38761761090.50 rows=3000608 width=8)
   ->  Seq Scan on lineitem  (cost=1.00..100213649.15  
rows=6001215 width=8)
   ->  Index Scan using oindex2 on myorders  (cost=0.00..6442.27  
rows=1 width=4)

 Index Cond: ("outer".l_orderkey = myorders.o_orderkey)
 Filter: (subplan)
 SubPlan
   ->  Index Scan using cnation on customer   
(cost=0.00..12859.39 rows=5251 width=0)

 Index Cond: (c_nationkey = 10)

How is the subplan handled by postgres at index level ? Is any sort  
of hashing done ?


This is better asked on pgsql-general... but the subplan does exactly  
what it says; an index scan. It will be executed for every row of the  
calling query.

--
Decibel!, aka Jim C. Nasby, Database Architect  [EMAIL PROTECTED]
Give your computer some brain candy! www.distributed.net Team #1828




smime.p7s
Description: S/MIME cryptographic signature

[HACKERS] Doubt in index subplan query

[Suresh]

Hello,

I have a query plan for a certain query

 Nested Loop  (cost=1.00..38761761090.50 rows=3000608 width=8)
   ->  Seq Scan on lineitem  (cost=1.00..100213649.15 rows=6001215 
width=8)
   ->  Index Scan using oindex2 on myorders  (cost=0.00..6442.27 rows=1 width=4)
 Index Cond: ("outer".l_orderkey = myorders.o_orderkey)
 Filter: (subplan)
 SubPlan
   ->  Index Scan using cnation on customer  (cost=0.00..12859.39 
rows=5251 width=0)
 Index Cond: (c_nationkey = 10)

How is the subplan handled by postgres at index level ? Is any sort of hashing 
done ?

Thanks and regards,
Suresh




  

Re: [HACKERS] Doubt in index scan code

[Dave Cramer]

On 8-Mar-08, at 11:06 AM, Suresh wrote:


Hello all,

I have a custom code written inside postgres in an application we use.
The snippet is as below :
Here inner plan is an index scan.

  scandesc = ((IndexScanState *)innerPlan)->iss_ScanDesc;

  flag=index_getmulti(scandesc, &tidelm->tid, 1, &ret_tids);

Now consider a query like

explain  select * from dept,manager where did=id ;
QUERY PLAN
---
 Nested Loop  (cost=0.00..269.09 rows=45 width=72)
   -> seq scan on manager  (cost=0.00..6.50 rows=45 width=36)
   ->  Index Scan using id1 on dept  (cost=0.00..5.82 rows=1 width=36)
 Index Cond: ("outer".did = dept.id)

Say seq scan retrieves did in the order 30,10, 20.. My doubt is in  
what order

will index_getmulti return tids. How does the scandesc work ?

Will it return the tids as firstly macthing inners for dept=30, then  
dept=10 ?


since you have no order by clause in the query rows will be returned  
in the order they are found on the disc.


Dave

Please help me with this.

Thanks and regards,
Suresh











Be a better friend, newshound, and know-it-all with Yahoo! Mobile.  
Try it now.

[HACKERS] Doubt in index scan code

[Suresh]

Hello all,

I have a custom code written inside postgres in an application we use.
The snippet is as below :
Here inner plan is an index scan.

  scandesc = ((IndexScanState *)innerPlan)->iss_ScanDesc; 

  flag=index_getmulti(scandesc, &tidelm->tid, 1, &ret_tids);

Now consider a query like

explain  select * from dept,manager where did=id ;
QUERY PLAN 
---
 Nested Loop  (cost=0.00..269.09 rows=45 width=72)
   -> seq scan on manager  (cost=0.00..6.50 rows=45 width=36)
   ->  Index Scan using id1 on dept  (cost=0.00..5.82 rows=1 width=36)
 Index Cond: ("outer".did = dept.id)

Say seq scan retrieves did in the order 30,10, 20.. My doubt is in what order 
will index_getmulti return tids. How does the scandesc work ? 

Will it return the tids as firstly macthing inners for dept=30, then dept=10 ?

Please help me with this.

Thanks and regards,
Suresh











   
-
Be a better friend, newshound, and know-it-all with Yahoo! Mobile.  Try it now.

Re: [HACKERS] Doubt in heap_release_fetch

[Tom Lane]

Suresh <[EMAIL PROTECTED]> writes:
> What is the time qualification check ?

HeapTupleSatisfiesVisibility().  See
src/include/utils/tqual.h
src/backend/utils/time/tqual.c
and if none of this is making any sense maybe you need to start here:
http://developer.postgresql.org/pgdocs/postgres/mvcc.html

regards, tom lane

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

[HACKERS] Doubt in heap_release_fetch

[Suresh]

Hello,

What do the following lines mean :

  /* Tuple failed time qual, but maybe caller wants to see it anyway. */
if (keep_buf)
*userbuf = buffer;
else
{
ReleaseBuffer(buffer);
*userbuf = InvalidBuffer;
}

What is the time qualification check ?

Thanks,
Suresh



   
-
Never miss a thing.   Make Yahoo your homepage.

Re: [HACKERS] Doubt in IndexScanDescData

[Gregory Stark]

One thing you might be missing is that indexes are relations too. They're a
bit different than heap relations (ie "tables") but they share enough
structure that it's worth using the same datatypes to represent both.

-- 
  Gregory Stark
  EnterpriseDB  http://www.enterprisedb.com
  Ask me about EnterpriseDB's RemoteDBA services!

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [HACKERS] Doubt in IndexScanDescData

[Hans-Juergen Schoenig]

take a look at that ...

http://www.postgresql.org/docs/8.3/static/indexam.html

this might clear up the problem.
here is an example making clear what happens:

select phone_number from phonebook where name = 'xy';

index is asked to find the right place in the heap to retrieve the data.
this is what happens during an index scan.
i suggest to step tnrough this process with a debugger to see what is  
going on.


hans



On Feb 17, 2008, at 5:13 PM, Suresh wrote:


Hans-Juergen Schoenig <[EMAIL PROTECTED]> wrote:

On Feb 17, 2008, at 4:33 PM, Suresh wrote:


[ "include/access/relscan.h" ]

In  IndexScanDescData,  whats the purpose of having two Relation  
variables.


typedef struct IndexScanDescData
{
RelationheapRelation;   /* heap relation  
descriptor, or NULL */
RelationindexRelation;  /* index relation  
descriptor */


...
}IndexScanDescData;




The index does not contain the entire tuple. If you index column A  
the index will not contain values in column B of the same table.
Thus, if you find a record in the index one of the things which  
have to be done is to get the record from disk to check visibility  
and other columns.


Yes thats correct. But I still dont get it. To get record from the  
disk on match, we need Relation data. But whats the purpose having  
two seperate Relation variables ?


Does it mean that heaprelation will contain only info about that  
particular column of the table and index relation will have info  
about the whole tuple of the relation ?



best regards,
hans-juergen schoenig



--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql.at, www.cybertec.at




Never miss a thing. Make Yahoo your homepage.




--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql.at, www.cybertec.at

Re: [HACKERS] Doubt in IndexScanDescData

[Suresh]

Hans-Juergen Schoenig <[EMAIL PROTECTED]> wrote:  
On Feb 17, 2008, at 4:33 PM, Suresh wrote:

[ "include/access/relscan.h" ]

In  IndexScanDescData,  whats the purpose of having two Relation variables.

typedef struct IndexScanDescData
{
RelationheapRelation;   /* heap relation descriptor, or NULL */
RelationindexRelation;  /* index relation descriptor */

...
}IndexScanDescData;





The index does not contain the entire tuple. If you index column A the index 
will not contain values in column B of the same table.
Thus, if you find a record in the index one of the things which have to be done 
is to get the record from disk to check visibility and other columns.


Yes thats correct. But I still dont get it. To get record from the disk on 
match, we need Relation data. But whats the purpose having two seperate 
Relation variables ?

Does it mean that heaprelation will contain only info about that particular 
column of the table and index relation will have info about the whole tuple of 
the relation ?


best regards,

hans-juergen schoenig






--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql.at, www.cybertec.at

 



   
-
Never miss a thing.   Make Yahoo your homepage.

Re: [HACKERS] Doubt in IndexScanDescData

[Hans-Juergen Schoenig]

On Feb 17, 2008, at 4:33 PM, Suresh wrote:


[ "include/access/relscan.h" ]

In  IndexScanDescData,  whats the purpose of having two Relation  
variables.


typedef struct IndexScanDescData
{
RelationheapRelation;   /* heap relation  
descriptor, or NULL */
RelationindexRelation;  /* index relation  
descriptor */


...
}IndexScanDescData;




The index does not contain the entire tuple. If you index column A  
the index will not contain values in column B of the same table.
Thus, if you find a record in the index one of the things which have  
to be done is to get the record from disk to check visibility and  
other columns.


best regards,

hans-juergen schoenig



--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql.at, www.cybertec.at

[HACKERS] Doubt in IndexScanDescData

[Suresh]

[ "include/access/relscan.h" ]

In  IndexScanDescData,  whats the purpose of having two Relation variables.

typedef struct IndexScanDescData
{
RelationheapRelation;   /* heap relation descriptor, or NULL */
RelationindexRelation;  /* index relation descriptor */

...
}IndexScanDescData;



   
-
Be a better friend, newshound, and know-it-all with Yahoo! Mobile.  Try it now.

Re: [HACKERS] doubt

[Hannu Krosing]

Ühel kenal päeval, N, 2007-07-12 kell 14:00, kirjutas Hannu Krosing:
> Ühel kenal päeval, K, 2007-07-11 kell 19:08, kirjutas Greg Smith:
> > On Wed, 11 Jul 2007, Narasimha Rao P.A wrote:
> > 
> > > Does postgreSQL support distributive query processing
> > 
> > Not internally.  It's possible in some situations to split queries up 
> > across multiple nodes using add-on software.  pgpool-II, available at 
> > http://pgfoundry.org/projects/pgpool/ provides an implementation of 
> > distributed queries if your table has a type of key such that you split 
> > across it, but it's relatively immature software 
> 
> Actually it is not "immature" at all, it is used 24/7 doing tens of
> thousands of queries per second :P

uups, I thought you were talking about plproxy ver.2 

http://pgfoundry.org/projects/plproxy

all my ranting is about it, not pgpool II, which is indeed quite new
project ;)

> But it is limited (by design) in what it can do - it is meant to run a
> postgresql _function_ on one or more db hosts based on parameter
> hash(es).
> 
> This can be used as a tool to construct a system which does distributed
> queries, and also to distribute load on small OLTP queries over a set of
> databases.
> 
> > and you would have to 
> > look at it very carefully to see if that parallel query implementation 
> > could fit your needs.
> 
> It has no parallel query implementation by itself (other than a special
> running the same SQL on a set of hosts and merging the results), but you
> definitely can progrem on using pgpool.
> 
> If you need something to distribute queries over a number of hosts
> automatically, there is a commercial data warehousing product (based on
> postgresql) available from greenplum, which does exactly this:
> 
> http://www.greenplum.com/index.php?page=greenplum-database
> 
> 
> > --
> > * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD
> > 
> > ---(end of broadcast)---
> > TIP 6: explain analyze is your friend
> 
> 
> ---(end of broadcast)---
> TIP 6: explain analyze is your friend


---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [HACKERS] doubt

[Hannu Krosing]

Ühel kenal päeval, K, 2007-07-11 kell 19:08, kirjutas Greg Smith:
> On Wed, 11 Jul 2007, Narasimha Rao P.A wrote:
> 
> > Does postgreSQL support distributive query processing
> 
> Not internally.  It's possible in some situations to split queries up 
> across multiple nodes using add-on software.  pgpool-II, available at 
> http://pgfoundry.org/projects/pgpool/ provides an implementation of 
> distributed queries if your table has a type of key such that you split 
> across it, but it's relatively immature software 

Actually it is not "immature" at all, it is used 24/7 doing tens of
thousands of queries per second :P

But it is limited (by design) in what it can do - it is meant to run a
postgresql _function_ on one or more db hosts based on parameter
hash(es).

This can be used as a tool to construct a system which does distributed
queries, and also to distribute load on small OLTP queries over a set of
databases.

> and you would have to 
> look at it very carefully to see if that parallel query implementation 
> could fit your needs.

It has no parallel query implementation by itself (other than a special
running the same SQL on a set of hosts and merging the results), but you
definitely can progrem on using pgpool.

If you need something to distribute queries over a number of hosts
automatically, there is a commercial data warehousing product (based on
postgresql) available from greenplum, which does exactly this:

http://www.greenplum.com/index.php?page=greenplum-database


> --
> * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD
> 
> ---(end of broadcast)---
> TIP 6: explain analyze is your friend


---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] doubt

[Greg Smith]

On Wed, 11 Jul 2007, Narasimha Rao P.A wrote:


Does postgreSQL support distributive query processing


Not internally.  It's possible in some situations to split queries up 
across multiple nodes using add-on software.  pgpool-II, available at 
http://pgfoundry.org/projects/pgpool/ provides an implementation of 
distributed queries if your table has a type of key such that you split 
across it, but it's relatively immature software and you would have to 
look at it very carefully to see if that parallel query implementation 
could fit your needs.


--
* Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] doubt

[Joshua D. Drake]

Narasimha Rao P.A wrote:

Does postgreSQL support distributive query processing


No.



Get the freedom to save as many mails as you wish. Click here to know 
how. 
 




--

  === The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive  PostgreSQL solutions since 1997
 http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/


---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly

[HACKERS] doubt

[Narasimha Rao P.A]

Does postgreSQL support distributive query processing

   
-
 Get the freedom to save as many mails as you wish. Click here to know how.

Re: [HACKERS] Doubt in parser

[Tom Lane]

Michael Glaesemann <[EMAIL PROTECTED]> writes:
> On Feb 16, 2006, at 21:37 , Dhanaraj wrote:
>> currently i looking at the postgres src code. I saw the scanner and  
>> parser implemetations at two different places (src/backend/parser/   
>> and  /src/bakend/bootstrp). Can anybody tell me the purpose of  
>> having two phases?? or will this help to parse the queries at  
>> different levels?

> AFAIK, I don't think the code is exactly the same (though I haven't  
> checked).

No, not even close.  The bootstrap parser reads the "bki" language
defined here:
http://developer.postgresql.org/docs/postgres/bki.html

"bki" is simple enough that it's hardly even worth using a bison parser
for, but someone did it that way so that's what we've got.

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org

Re: [HACKERS] Doubt in parser

[Michael Glaesemann]

On Feb 16, 2006, at 21:37 , Dhanaraj wrote:


hi

currently i looking at the postgres src code. I saw the scanner and  
parser implemetations at two different places (src/backend/parser/   
and  /src/bakend/bootstrp). Can anybody tell me the purpose of  
having two phases?? or will this help to parse the queries at  
different levels?


AFAIK, I don't think the code is exactly the same (though I haven't  
checked). The bootstrap code is used to get the PostgreSQL server  
started: there is a considerable amount of information stored in the  
system catalogs that the server needs to use. The server needs access  
to a scanner/parser to be able to read this information. The  
bootstrap code provides the server with enough knowledge to get  
started. The backend parser and scanner is more feature-filled.


Someone please feel free to step in and correct me if I'm off base :)

Michael Glaesemann
grzm myrealbox com




---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] Doubt in parser

[Alvaro Herrera]

Dhanaraj wrote:
> hi
> 
> currently i looking at the postgres src code. I saw the scanner and 
> parser implemetations at two different places (src/backend/parser/  and  
> /src/bakend/bootstrp). Can anybody tell me the purpose of having two 
> phases?? or will this help to parse the queries at different levels?

The bootstrap parser is using only in bootstrap mode, which is when the
template1 database is initially created.  It has a completely different
syntax than the main parser.

If what you are looking for is to implement a new command or modify an
existing one, ignore the bootstrap parser.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [HACKERS] Doubt in parser

[Martijn van Oosterhout]

On Thu, Feb 16, 2006 at 06:07:25PM +0530, Dhanaraj wrote:
> hi
> 
> currently i looking at the postgres src code. I saw the scanner and 
> parser implemetations at two different places (src/backend/parser/  and  
> /src/bakend/bootstrp). Can anybody tell me the purpose of having two 
> phases?? or will this help to parse the queries at different levels?

The first one is the actual parser for queries you send. The latter is
the bootstrap parser which is only used during the inital bootstrap of
a database. It needs to be seperate because of things like the names of
columns are stored in a pg_attribute, yet how can you fill the table if
you don't know what the columns are called.

The latter is basically a glorified data loader to handle this special
case. It can't do queries or anything like that. You can basically
ignore it for normal development.

Have a nice day,
-- 
Martijn van Oosterhout  http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.


signature.asc
Description: Digital signature

[HACKERS] Doubt in parser

[Dhanaraj]

hi

currently i looking at the postgres src code. I saw the scanner and 
parser implemetations at two different places (src/backend/parser/  and  
/src/bakend/bootstrp). Can anybody tell me the purpose of having two 
phases?? or will this help to parse the queries at different levels?


Thanks
Dhanaraj

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org

Re: [HACKERS] Doubt

[Simon Riggs]

On Fri, 2005-11-25 at 15:24 -0300, Gustavo Tonini wrote:
> What is "ISTM"?

Google tells me it stands for The Irish Society for Travel Medicine, but
it also gives a few other suggestions.

ISTM that google has the answer. :-)

Best Regards, Simon Riggs


---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] Doubt

[Pollard, Mike]

It Seems To Me.

 

Here’s a decent list of common
acronyms:

 

http://www.fun-with-words.com/acronyms.html

 



Mike Pollard

SUPRA Server SQL Engineering and Support

Cincom Systems, Inc.



 Better to
remain silent and be thought a fool than to speak out and remove all doubt.


Abraham Lincoln



-Original Message-
From: [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED] On
Behalf Of Gustavo Tonini
Sent: Friday, November 25, 2005
1:24 PM
To: pgsql-hackers@postgresql.org
Subject: [HACKERS] Doubt

 

What is "ISTM"?

Sorry,
Gustavo.

[HACKERS] Doubt

[Gustavo Tonini]

What is "ISTM"?

Sorry,
Gustavo.

Re: [HACKERS] Doubt w.r.t vacuum

[Shridhar Daithankar]

On 28 Jul 2003 at 9:56, Alvaro Herrera wrote:

> On Mon, Jul 28, 2003 at 02:29:36PM +0530, Shridhar Daithankar wrote:
> 
> > I was just wondering over it. This is for difference between vacuum full and 
> > vacuum analyze. Can somebody enlighten,
> 
> Actually, the different concepts are "lazy vacuum" (plain VACUUM
> command, with or without ANALYZE) and full vacuum ("VACUUM FULL"
> command, with or without ANALYZE).
> 
> Lazy vacuum works one page at a time, so it doesn't need to lock the
> entire table.  It is able to recover empty space from both updated and
> deleted tuples -- in fact, they look the same to it.  All free space on
> each page is defragmented.  Pages with free space are recorded in the
> Free Space Map.  The FSM has limited space available, so only the pages
> with the most free space will be recorded.
> 
> Vacuum full locks the entire table and moves tuples between pages.  It
> leaves all pages full of tuples (except, obviously, the last one), so it
> doesn't need to record them in the FSM.  Pages that are empty at the end
> of the table are truncated.  This was the only version of VACUUM present
> in releases previous to 7.2.

OK. So here is my interpretation,

Vacuum full reclaims the space that is spilled to disk due to insufficient 
vacuumi analyze and/or inadequate FSM size.

So to keep your database free from fat, use adequate FSM and use a autovacuum 
daemon..

Am I going overboard here?

Bye
 Shridhar

--
system-independent, adj.:   Works equally poorly on all systems.


---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])

Re: [HACKERS] Doubt w.r.t vacuum

[Robert Treat]

On Mon, 2003-07-28 at 11:04, Tom Lane wrote:
> Alvaro Herrera <[EMAIL PROTECTED]> writes:
> > Vacuum full locks the entire table and moves tuples between pages.  It
> > leaves all pages full of tuples (except, obviously, the last one), so it
> > doesn't need to record them in the FSM.
> 
> This is overoptimistic :-(.  VACUUM FULL cannot necessarily compact the
> table completely, and so it will record free space in FSM (if there is
> any worth recording).  An example situation is that page 1000 may
> contain a very large tuple, which will not fit on any earlier page.
> Once VACUUM FULL discovers this fact, it will not bother shuffling
> tuples on earlier pages, since it's not going to be able to truncate the
> table to less than 1000 pages.  There may nonetheless be enough space
> available in earlier pages to store thousands of smaller-sized tuples.
> 

Isn't it possible that the reshuffling of tuples before page 1000 could
open up enough space to move the overly large tuple?

Robert Treat
-- 
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL


---(end of broadcast)---
TIP 8: explain analyze is your friend

Re: [HACKERS] Doubt w.r.t vacuum

[Tom Lane]

Robert Treat <[EMAIL PROTECTED]> writes:
>> This is overoptimistic :-(.  VACUUM FULL cannot necessarily compact the
>> table completely, and so it will record free space in FSM (if there is
>> any worth recording).  An example situation is that page 1000 may
>> contain a very large tuple, which will not fit on any earlier page.

> Isn't it possible that the reshuffling of tuples before page 1000 could
> open up enough space to move the overly large tuple?

Not in the same vacuum pass.  Reshuffling opens *zero* space until you
commit the shuffling transaction, because you can't destroy the old
copies until you commit the moved ones.

You could imagine making multiple passes, but at that point it's almost
certainly faster to forget the VACUUM FULL approach entirely, and do
something more like CLUSTER: copy all the live tuples into a new file.

regards, tom lane

---(end of broadcast)---
TIP 6: Have you searched our list archives?

   http://archives.postgresql.org

Re: [HACKERS] Doubt w.r.t vacuum

[Tom Lane]

Alvaro Herrera <[EMAIL PROTECTED]> writes:
> Vacuum full locks the entire table and moves tuples between pages.  It
> leaves all pages full of tuples (except, obviously, the last one), so it
> doesn't need to record them in the FSM.

This is overoptimistic :-(.  VACUUM FULL cannot necessarily compact the
table completely, and so it will record free space in FSM (if there is
any worth recording).  An example situation is that page 1000 may
contain a very large tuple, which will not fit on any earlier page.
Once VACUUM FULL discovers this fact, it will not bother shuffling
tuples on earlier pages, since it's not going to be able to truncate the
table to less than 1000 pages.  There may nonetheless be enough space
available in earlier pages to store thousands of smaller-sized tuples.

regards, tom lane

---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])

Re: [HACKERS] Doubt w.r.t vacuum

[Tom Lane]

"Shridhar Daithankar" <[EMAIL PROTECTED]> writes:
> 1. IIRC vacuum recovers/reuses dead tuples generated from update but can not do 
> so for delete? Why?

This is not correct.

> 2. Vacuum full locks entire table, is it possible that it locks a page at a 
> time and deal with it.

No.  You can't compact the table by moving tuples without locking the
entire table.  (For example, if we move a tuple from the end down to an
earlier page, it's quite possible that a concurrently executing
sequential scan would miss that tuple entirely.  Another problem is that
we cannot truncate the table to fewer pages without locking out writers;
else we may decide that there are N empty pages, then execute ftruncate()
just after someone has put a new tuple into one of those pages.)

Non-full vacuum is designed specifically to do what can be done without
an exclusive lock.

regards, tom lane

---(end of broadcast)---
TIP 6: Have you searched our list archives?

   http://archives.postgresql.org

Re: [HACKERS] Doubt w.r.t vacuum

[Alvaro Herrera]

On Mon, Jul 28, 2003 at 02:29:36PM +0530, Shridhar Daithankar wrote:

> I was just wondering over it. This is for difference between vacuum full and 
> vacuum analyze. Can somebody enlighten,

Actually, the different concepts are "lazy vacuum" (plain VACUUM
command, with or without ANALYZE) and full vacuum ("VACUUM FULL"
command, with or without ANALYZE).

Lazy vacuum works one page at a time, so it doesn't need to lock the
entire table.  It is able to recover empty space from both updated and
deleted tuples -- in fact, they look the same to it.  All free space on
each page is defragmented.  Pages with free space are recorded in the
Free Space Map.  The FSM has limited space available, so only the pages
with the most free space will be recorded.

Vacuum full locks the entire table and moves tuples between pages.  It
leaves all pages full of tuples (except, obviously, the last one), so it
doesn't need to record them in the FSM.  Pages that are empty at the end
of the table are truncated.  This was the only version of VACUUM present
in releases previous to 7.2.

If I got something wrong, I'm sure someone will correct me.

-- 
Alvaro Herrera ()
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)

---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])

Re: [HACKERS] Doubt w.r.t vacuum

[Shridhar Daithankar]

On 28 Jul 2003 at 9:11, Doug McNaught wrote:

> "Shridhar Daithankar" <[EMAIL PROTECTED]> writes:
> 
> > Hi,
> > 
> > I was just wondering over it. This is for difference between vacuum full and 
> > vacuum analyze. Can somebody enlighten,
> > 
> > 1. IIRC vacuum recovers/reuses dead tuples generated from update but can not do 
> > so for delete? Why?
> 
> YDNRC.

You did not read... C for what? Code?

> 
> > 2. Vacuum full locks entire table, is it possible that it locks a
> > page at a time and deal with it. It will make vacuum full
> > non-blocking at the cost of letting it run for a longer time. Or is
> > it that the defragmentation algorithm needs more than a page?
> 
> This I don't know, but I imagine that if what you suggest was easy to
> do it would have been done, and there would have been no need for two
> different kinds of VACUUM.

I went thr. the code, although vbery briefly but I can imagine that code being 
dependent upon tons of other things. Didn't understand everything so left it as 
it is..
Bye
 Shridhar

--
Mix's Law:  There is nothing more permanent than a temporary building.  There 
is 
nothing more permanent than a temporary tax.


---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly

Re: [HACKERS] Doubt w.r.t vacuum

[Doug McNaught]

"Shridhar Daithankar" <[EMAIL PROTECTED]> writes:

> Hi,
> 
> I was just wondering over it. This is for difference between vacuum full and 
> vacuum analyze. Can somebody enlighten,
> 
> 1. IIRC vacuum recovers/reuses dead tuples generated from update but can not do 
> so for delete? Why?

YDNRC.

> 2. Vacuum full locks entire table, is it possible that it locks a
> page at a time and deal with it. It will make vacuum full
> non-blocking at the cost of letting it run for a longer time. Or is
> it that the defragmentation algorithm needs more than a page?

This I don't know, but I imagine that if what you suggest was easy to
do it would have been done, and there would have been no need for two
different kinds of VACUUM.

-DOUG

---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster

[HACKERS] Doubt w.r.t vacuum

[Shridhar Daithankar]

Hi,

I was just wondering over it. This is for difference between vacuum full and 
vacuum analyze. Can somebody enlighten,

1. IIRC vacuum recovers/reuses dead tuples generated from update but can not do 
so for delete? Why?

2. Vacuum full locks entire table, is it possible that it locks a page at a 
time and deal with it. It will make vacuum full non-blocking at the cost of 
letting it run for a longer time. Or is it that the defragmentation algorithm 
needs more than a page?

Just a thought..


Bye
 Shridhar

--
Weed's Axiom:   Never ask two questions in a business letter.   The reply will 
discuss the one in which you areleast interested and say nothing about the 
other.


---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings