Subject: Re: [Patch] Temporary tables that do not bloat pg_catalog
 (a.k.a fast temp tables)
To: Aleksander Alekseev <a.alekseev@postgrespro.ru>
References: <20160729141552.4062e19b@fujitsu>
 <39b0f423-a4df-3564-3b4a-b1de6f9eea80@2ndquadrant.com>
 <CAFj8pRAv+vPxzTQMviCsYqQrfpYei5bTvX80zRUp1Q3xAbTWyg@mail.gmail.com>
 <1184.1469890030@sss.pgh.pa.us>
 <5cdc3fde-d6a5-f0d2-b930-485bdcaecde5@2ndquadrant.com>
 <20160801124531.66cde374@fujitsu>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Pavel Stehule <pavel.stehule@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 David Steele <david@pgmasters.net>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <26c3bc60-6613-e7ab-517a-53136f01fb6c@2ndquadrant.com>
Date: Mon, 1 Aug 2016 15:09:01 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101
 Thunderbird/46.0a2
MIME-Version: 1.0
In-Reply-To: <20160801124531.66cde374@fujitsu>
Content-Type: multipart/mixed;
 boundary="------------2E31B374BF89ADC007CBF6DE"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------2E31B374BF89ADC007CBF6DE
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 08/01/2016 11:45 AM, Aleksander Alekseev wrote:
> Hello.
>
> Thanks everyone for great comments!
>
>> Well, jokes aside, that's a pretty lousy excuse for not writing any
>> docs
>
> I think maybe I put it in a wrong way. There are currently a lot of
> comments in a code, more then enough to understand how this feature
> works. What I meant is that this is not a final version of a patch and
> a few paragraphs are yet to be written. At least it's how I see it. If
> you believe that some parts of the code are currently hard to understand
> and some comments could be improved, please name it and I will be happy
> to fix it.

I don't think there's "a lot of comments in the code", not even 
remotely. At least not in the files I looked into - heapam, indexam, 
xact etc. There are a few comments in general, and most of them only 
comment obvious facts, like "ignore in-memory tuples" right before a 
trivial if statement.

What is needed is an overview of the approach, so that the reviewers can 
read that first, instead of assembling the knowledge from pieces 
scattered over comments in many pieces. But I see the fasttab.c contains 
this:

/* TODO TODO comment the general idea - in-memory tuples and indexes, 
hooks principle, FasttabSnapshots, etc */

The other thing that needs to happen is you need to modify comments in 
front of some of the modified methods - e.g. the comments may need a 
paragraph "But when the table is fast temporary, what happens is ..."

>
>> IMHO if this patch gets in, we should use it as the only temp table
>> implementation (Or can you think of cases where keeping rows in
>> pg_class has advantages?). That'd also eliminate the need for FAST
>> keyword in the CREATE TABLE command.
>
>> Probably has zero value to have slow and fast temp tables (from
>> catalogue cost perspective). So the FAST implementation should be used
>> everywhere.
>
> If there are no objections I see no reason no to do it in a next
> version of a patch.

I believe there will be a lot of discussion about this.

>
>> I'm getting failures in the regression suite
>
> I've run regression suite like 10 times in a row in different
> environments with different build flags but didn't manage to reproduce
> it. Also our DBAs are testing this feature for weeks now on real-world
> applications and they didn't report anything like this. Could you
> please describe how to reproduce this issue?
>

Nothing special:

$ ./configure --prefix=/home/user/pg-temporary --enable-debug \
   --enable-cassert

$ make -s clean && make -s -j4 install

$ export PATH=/home/user/pg-temporary/bin:$PATH

$ pg_ctl -D ~/tmp/data-temporary init

$ pg_ctl -D ~/tmp/data-temporary -l ~/temporary.log start

$ make installcheck

I get the failures every time - regression diff attached. The first 
failure in "rolenames" is expected, because of clash with existing user 
name. The remaining two failures are not.

I only get the failure for "installcheck" but not "check" for some reason.

regards

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

--------------2E31B374BF89ADC007CBF6DE
Content-Type: text/plain; charset=UTF-8;
 name="regression.diffs"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="regression.diffs"

*** /home/user/work/postgres/src/test/regress/expected/rolenames.out	2016-07-18 20:48:39.421444507 +0200
--- /home/user/work/postgres/src/test/regress/results/rolenames.out	2016-08-01 15:07:04.597000000 +0200
***************
*** 42,47 ****
--- 42,48 ----
  CREATE ROLE "current_user";
  CREATE ROLE "session_user";
  CREATE ROLE "user";
+ ERROR:  role "user" already exists
  CREATE ROLE current_user; -- error
  ERROR:  CURRENT_USER cannot be used as a role name here
  LINE 1: CREATE ROLE current_user;
***************
*** 956,958 ****
--- 957,960 ----
  DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
  DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
  DROP ROLE "Public", "None", "current_user", "session_user", "user";
+ ERROR:  current user cannot be dropped

======================================================================


--------------2E31B374BF89ADC007CBF6DE
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

--------------2E31B374BF89ADC007CBF6DE--