Обсуждение: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

От:
Pavan Deolasee
Дата:

Hi,

Jeff Janes raised an issue [1] about PD_ALL_VISIBLE not being set correctly while loading data via COPY FREEZE and had also posted a draft patch.

I now have what I think is a more complete patch. I took a slightly different approach and instead of setting PD_ALL_VISIBLE bit initially and then not clearing it during insertion, we now recheck the page for all-frozen, all-visible tuples just before switching to a new page. This allows us to then also mark set the visibility map bit, like we do in vacuumlazy.c

Some special treatment is required to handle the last page before bulk insert it shutdown. We could have chosen not to do anything special for the last page and let it remain unfrozen, but I thought it makes sense to take that extra effort so that we can completely freeze the table and set all VM bits at the end of COPY FREEZE.

Let me know what you think.

Thanks,
Pavan  


--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Kuntal Ghosh
Дата:

Hello Pavan,

Thank you for the patch. It seems to me that while performing COPY
FREEZE, if we've copied tuples in a previously emptied page, we can
set the PageSetAllVisible(page) in heap_muli_insert only. Something
like,

bool init = (ItemPointerGetOffsetNumber(&(heaptuples[ndone]->t_self))
== FirstOffsetNumber &&
         PageGetMaxOffsetNumber(page) == FirstOffsetNumber + nthispage - 1);
if (init && (options & HEAP_INSERT_FROZEN))
 PageSetAllVisible(page);

Later, you can skip the pages for
CheckAndSetAllVisibleBulkInsertState() where PD_ALL_VISIBLE flag is
already set. Do you think it's correct?


On Thu, Feb 21, 2019 at 11:35 AM Pavan Deolasee
<> wrote:
>
> Hi,
>
> Jeff Janes raised an issue [1] about PD_ALL_VISIBLE not being set correctly while loading data via COPY FREEZE and
hadalso posted a draft patch. 
>
> I now have what I think is a more complete patch. I took a slightly different approach and instead of setting
PD_ALL_VISIBLEbit initially and then not clearing it during insertion, we now recheck the page for all-frozen,
all-visibletuples just before switching to a new page. This allows us to then also mark set the visibility map bit,
likewe do in vacuumlazy.c 
>
> Some special treatment is required to handle the last page before bulk insert it shutdown. We could have chosen not
todo anything special for the last page and let it remain unfrozen, but I thought it makes sense to take that extra
effortso that we can completely freeze the table and set all VM bits at the end of COPY FREEZE. 
>
> Let me know what you think.
>
> Thanks,
> Pavan
>
> [1] https://www.postgresql.org/message-id/CAMkU%3D1w3osJJ2FneELhhNRLxfZitDgp9FPHee08NT2FQFmz_pQ%40mail.gmail.com
>
> --
>  Pavan Deolasee                   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services



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


От:
Simon Riggs
Дата:

On Thu, 21 Feb 2019 at 15:38, Kuntal Ghosh <> wrote:
 
Thank you for the patch. It seems to me that while performing COPY
FREEZE, if we've copied tuples in a previously emptied page

There won't be any previously emptied pages because of the pre-conditions required for FREEZE. 

--
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
От:
Kuntal Ghosh
Дата:

On Tue, Feb 26, 2019 at 6:46 PM Simon Riggs <> wrote:
>
> On Thu, 21 Feb 2019 at 15:38, Kuntal Ghosh <> wrote:
>
>>
>> Thank you for the patch. It seems to me that while performing COPY
>> FREEZE, if we've copied tuples in a previously emptied page
>
>
> There won't be any previously emptied pages because of the pre-conditions required for FREEZE.
>
Right, I missed that part. Thanks for pointing that out. But, this
optimization is still possible for copying frozen tuples in a newly
created page, right? If current backend allocates a new page, copies a
bunch of frozen tuples in that page, it can set the PD_ALL_VISIBLE in
the same operation.



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


От:
Jeff Janes
Дата:

On Thu, Feb 21, 2019 at 1:05 AM Pavan Deolasee <> wrote:
Hi,

Jeff Janes raised an issue [1] about PD_ALL_VISIBLE not being set correctly while loading data via COPY FREEZE and had also posted a draft patch.

I now have what I think is a more complete patch. I took a slightly different approach and instead of setting PD_ALL_VISIBLE bit initially and then not clearing it during insertion, we now recheck the page for all-frozen, all-visible tuples just before switching to a new page. This allows us to then also mark set the visibility map bit, like we do in vacuumlazy.c

Some special treatment is required to handle the last page before bulk insert it shutdown. We could have chosen not to do anything special for the last page and let it remain unfrozen, but I thought it makes sense to take that extra effort so that we can completely freeze the table and set all VM bits at the end of COPY FREEZE.

Let me know what you think.

Hi Pavan, thanks for picking this up.

After doing a truncation and '\copy ... with (freeze)' of a table with long data, I find that the associated toast table has a handful of unfrozen blocks.  I don't know if that is an actual problem, but it does seem a bit odd, and thus suspicious.

perl -le 'print join "", map rand(), 1..500 foreach 1..1000000' > foo

create table foobar1 (x text);
begin;
truncate foobar1;
\copy foobar1 from foo with (freeze)
commit;
select all_visible,all_frozen,pd_all_visible, count(*) from pg_visibility('pg_toast.pg_toast_25085') group by 1,2,3;
 all_visible | all_frozen | pd_all_visible |  count  
-------------+------------+----------------+---------
 f           | f          | f              |      18
 t           | t          | t              | 530,361
(2 rows)

Cheers,

Jeff 
От:
Pavan Deolasee
Дата:



On Wed, Feb 27, 2019 at 7:05 AM Jeff Janes <> wrote:


After doing a truncation and '\copy ... with (freeze)' of a table with long data, I find that the associated toast table has a handful of unfrozen blocks.  I don't know if that is an actual problem, but it does seem a bit odd, and thus suspicious.


Hi Jeff, thanks for looking at it and the test. I can reproduce the problem and quite curiously block number 1 and then every 32672th block is getting skipped.

postgres=# select * from pg_visibility('pg_toast.pg_toast_16384') where all_visible = 'f';
 blkno  | all_visible | all_frozen | pd_all_visible 
--------+-------------+------------+----------------
      1 | f           | f          | f
  32673 | f           | f          | f
  65345 | f           | f          | f
  98017 | f           | f          | f
 130689 | f           | f          | f
 163361 | f           | f          | f
 <snip>

Having investigated this a bit, I see that a relcache invalidation arrives after 1st and then after every 32672th block is filled. That clears the rel->rd_smgr field and we lose the information about the saved target block. The code then moves to extend the relation again and thus skips the previously less-than-half filled block, losing the free space in that block.

postgres=# SELECT * FROM page_header(get_raw_page('pg_toast.pg_toast_16384', 0));
    lsn     | checksum | flags | lower | upper | special | pagesize | version | prune_xid 
------------+----------+-------+-------+-------+---------+----------+---------+-----------
 1/15B37748 |        0 |     4 |    40 |    64 |    8192 |     8192 |       4 |         0
(1 row)

postgres=# SELECT * FROM page_header(get_raw_page('pg_toast.pg_toast_16384', 1));
    lsn     | checksum | flags | lower | upper | special | pagesize | version | prune_xid 
------------+----------+-------+-------+-------+---------+----------+---------+-----------
 1/15B39A28 |        0 |     4 |    28 |  7640 |    8192 |     8192 |       4 |         0
(1 row)

postgres=# SELECT * FROM page_header(get_raw_page('pg_toast.pg_toast_16384', 2));
    lsn     | checksum | flags | lower | upper | special | pagesize | version | prune_xid 
------------+----------+-------+-------+-------+---------+----------+---------+-----------
 1/15B3BE08 |        0 |     4 |    40 |    64 |    8192 |     8192 |       4 |         0
(1 row)

So the block 1 has a large amount of free space (upper - lower), which never gets filled.

I am not yet sure what causes the relcache invalidation at regular intervals. But if I have to guess, it could be because of a new VM (or FSM?) page getting allocated. I am bit puzzled because this issue seems to only occur with toast tables since I tested the patch while writing it on a regular table and did not see any block remaining unfrozen. I tested only upto 450 blocks, but that shouldn't matter because with your test, we see the problem with block 1 as well. So something to look into in more detail.

While we could potentially fix this by what you'd done in the original patch and what Kuntal also suggested, i.e. by setting the PD_ALL_VISIBLE bit during page initialisation itself, I am a bit circumspect about that approach for two reasons:

1. It requires us to then add extra logic to avoid clearing the bit during insertions
2. It requires us to also update the VM bit during page init or risk having divergent views on the page-level bit and the VM bit.

And even if we do that, this newly discovered problem of less-than-half filled intermediate blocks remain. I wonder if we should instead track the last used block in BulkInsertState and if the relcache invalidation flushes smgr, start inserting again from the last saved block. In fact, we already track the last used buffer in BulkInsertState and that's enough to know the last used block.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Masahiko Sawada
Дата:

On Thu, Feb 21, 2019 at 3:05 PM Pavan Deolasee <> wrote:
>
> Hi,
>
> Jeff Janes raised an issue [1] about PD_ALL_VISIBLE not being set correctly while loading data via COPY FREEZE and
hadalso posted a draft patch. 
>
> I now have what I think is a more complete patch. I took a slightly different approach and instead of setting
PD_ALL_VISIBLEbit initially and then not clearing it during insertion, we now recheck the page for all-frozen,
all-visibletuples just before switching to a new page. This allows us to then also mark set the visibility map bit,
likewe do in vacuumlazy.c 

I might be missing something but why do we need to recheck whether
each pages is all-frozen after insertion? I wonder if we can set
all-frozen without checking all tuples again in this case.

Regards,

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


От:
Pavan Deolasee
Дата:


On Mon, Mar 11, 2019 at 1:37 PM Masahiko Sawada <> wrote:


I might be missing something but why do we need to recheck whether
each pages is all-frozen after insertion? I wonder if we can set
all-frozen without checking all tuples again in this case.

It's possible that the user may have inserted unfrozen rows (via regular INSERT or COPY without FREEZE option) before inserting frozen rows. So we can't set the all-visible/all-frozen flag unconditionally. I also find it safer to do an explicit check to ensure we never accidentally mark a page as all-frozen. Since the check is performed immediately after the page becomes full and only once per page, there shouldn't be any additional IO cost and the check should be quite fast. 

Thanks,
Pavan
--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Masahiko Sawada
Дата:

On Tue, Mar 12, 2019 at 4:54 PM Pavan Deolasee <> wrote:
>
>
> On Mon, Mar 11, 2019 at 1:37 PM Masahiko Sawada <> wrote:
>>
>>
>>
>> I might be missing something but why do we need to recheck whether
>> each pages is all-frozen after insertion? I wonder if we can set
>> all-frozen without checking all tuples again in this case.
>
>
> It's possible that the user may have inserted unfrozen rows (via regular INSERT or COPY without FREEZE option) before
insertingfrozen rows. 

I think that since COPY FREEZE can be executed only when the table is
created or truncated within the transaction other users cannot insert
any rows during COPY FREEZE.

> So we can't set the all-visible/all-frozen flag unconditionally. I also find it safer to do an explicit check to
ensurewe never accidentally mark a page as all-frozen. Since the check is performed immediately after the page becomes
fulland only once per page, there shouldn't be any additional IO cost and the check should be quite fast. 

I'd suggest to measure performance overhead. I can imagine one use
case of COPY FREEZE is the loading a very large table. Since in
addition to set visibility map bits this patch could scan a very large
table I'm concerned that how much performance is degraded by this
patch.
Regards,

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


От:
Pavan Deolasee
Дата:



On Wed, Mar 13, 2019 at 11:37 AM Masahiko Sawada <> wrote:


I think that since COPY FREEZE can be executed only when the table is
created or truncated within the transaction other users cannot insert
any rows during COPY FREEZE.


Right. But the truncating transaction can insert unfrozen rows into the table before inserting more rows via COPY FREEZE.

postgres=# CREATE EXTENSION pageinspect ;
CREATE EXTENSION
postgres=# BEGIN;
BEGIN
postgres=# TRUNCATE testtab ;
TRUNCATE TABLE
postgres=# INSERT INTO testtab VALUES (100, 200);
INSERT 0 1
postgres=# COPY testtab FROM STDIN WITH (FREEZE);
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> 1 2
>> 2 3
>> \.
COPY 2
postgres=# COMMIT;

postgres=# SELECT lp, to_hex(t_infomask) FROM heap_page_items(get_raw_page('testtab', 0));
 lp | to_hex 
----+--------
  1 | 800
  2 | b00
  3 | b00
(3 rows)

The first row in inserted by regular insert and it's not frozen. The next 2 are frozen. We can't mark such as page all-visible, all-frozen.
 

I'd suggest to measure performance overhead. I can imagine one use
case of COPY FREEZE is the loading a very large table. Since in
addition to set visibility map bits this patch could scan a very large
table I'm concerned that how much performance is degraded by this
patch.

Ok. I will run some tests. But please note that this patch is a bug fix to address the performance issue that is caused by having to rewrite the entire table when all-visible bit is set on the page during first vacuum. So while we may do some more work during COPY FREEZE, we're saving a lot of page writes during next vacuum. Also, since the scan that we are doing in this patch is done on a page that should be in the buffer cache, we will pay a bit in terms of CPU cost, but not anything in terms of IO cost.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Masahiko Sawada
Дата:

On Thu, Mar 14, 2019 at 5:17 PM Pavan Deolasee <> wrote:
>
>
>
> On Wed, Mar 13, 2019 at 11:37 AM Masahiko Sawada <> wrote:
>>
>>
>>
>> I think that since COPY FREEZE can be executed only when the table is
>> created or truncated within the transaction other users cannot insert
>> any rows during COPY FREEZE.
>>
>
> Right. But the truncating transaction can insert unfrozen rows into the table before inserting more rows via COPY
FREEZE.
>
> postgres=# CREATE EXTENSION pageinspect ;
> CREATE EXTENSION
> postgres=# BEGIN;
> BEGIN
> postgres=# TRUNCATE testtab ;
> TRUNCATE TABLE
> postgres=# INSERT INTO testtab VALUES (100, 200);
> INSERT 0 1
> postgres=# COPY testtab FROM STDIN WITH (FREEZE);
> Enter data to be copied followed by a newline.
> End with a backslash and a period on a line by itself, or an EOF signal.
> >> 1 2
> >> 2 3
> >> \.
> COPY 2
> postgres=# COMMIT;
>
> postgres=# SELECT lp, to_hex(t_infomask) FROM heap_page_items(get_raw_page('testtab', 0));
>  lp | to_hex
> ----+--------
>   1 | 800
>   2 | b00
>   3 | b00
> (3 rows)
>
> The first row in inserted by regular insert and it's not frozen. The next 2 are frozen. We can't mark such as page
all-visible,all-frozen. 

Understood. Thank you for explanation!

>
>>
>>
>> I'd suggest to measure performance overhead. I can imagine one use
>> case of COPY FREEZE is the loading a very large table. Since in
>> addition to set visibility map bits this patch could scan a very large
>> table I'm concerned that how much performance is degraded by this
>> patch.
>
>
> Ok. I will run some tests. But please note that this patch is a bug fix to address the performance issue that is
causedby having to rewrite the entire table when all-visible bit is set on the page during first vacuum. So while we
maydo some more work during COPY FREEZE, we're saving a lot of page writes during next vacuum. Also, since the scan
thatwe are doing in this patch is done on a page that should be in the buffer cache, we will pay a bit in terms of CPU
cost,but not anything in terms of IO cost. 

Agreed. I had been misunderstanding this patch. The page scan during
COPY FREEZE is necessary and it's very cheaper than doing in the first
vacuum.

Regards,

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


От:
David Steele
Дата:

Hi Pavan,

On 3/14/19 2:20 PM, Masahiko Sawada wrote:
> On Thu, Mar 14, 2019 at 5:17 PM Pavan Deolasee <> wrote:
>>
>> Ok. I will run some tests. But please note that this patch is a bug fix to address the performance issue that is
causedby having to rewrite the entire table when all-visible bit is set on the page during first vacuum. So while we
maydo some more work during COPY FREEZE, we're saving a lot of page writes during next vacuum. Also, since the scan
thatwe are doing in this patch is done on a page that should be in the buffer cache, we will pay a bit in terms of CPU
cost,but not anything in terms of IO cost.
 
> 
> Agreed. I had been misunderstanding this patch. The page scan during
> COPY FREEZE is necessary and it's very cheaper than doing in the first
> vacuum.

I have removed Ibrar as a reviewer since there has been no review from 
them in three weeks, and too encourage others to have a look.

Regards,
-- 
-David



От:
Pavan Deolasee
Дата:



On Thu, Mar 14, 2019 at 3:54 PM Masahiko Sawada <> wrote:

>
>
> Ok. I will run some tests. But please note that this patch is a bug fix to address the performance issue that is caused by having to rewrite the entire table when all-visible bit is set on the page during first vacuum. So while we may do some more work during COPY FREEZE, we're saving a lot of page writes during next vacuum. Also, since the scan that we are doing in this patch is done on a page that should be in the buffer cache, we will pay a bit in terms of CPU cost, but not anything in terms of IO cost.

Agreed. I had been misunderstanding this patch. The page scan during
COPY FREEZE is necessary and it's very cheaper than doing in the first
vacuum.

Thanks for agreeing to the need of this bug fix. I ran some simple tests anyways and here are the results.

The test consists of a simple table with three columns, two integers and one char(100). I then ran COPY (FREEZE), loading 7M rows, followed by a VACUUM. The total size of the raw data is about 800MB and the table size in Postgres is just under 1GB. The results for 3 runs in milliseconds are:

Master:
 COPY FREEZE: 40243.725   40309.675    40783.836
 VACUUM: 2685.871  2517.445    2508.452 

Patched:
 COPY FREEZE: 40942.410  40495.303   40638.075
 VACUUM: 25.067  35.793   25.390

So there is a slight increase in the time to run COPY FREEZE, but a significant reduction in time to VACUUM the table. The benefits will only go up if the table is vacuumed much  later when most of the pages are already written to the disk and removed from shared buffers and/or kernel cache.

I hope this satisfies your doubts regarding performance implications of the patch.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Darafei Praliaskouski
Дата:

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

This patch is particularly helpful in processing OpenStreetMap Data in PostGIS.
OpenStreetMap is imported as a stream of 300-900 (depending on settings) gigabytes, that are needing a VACUUM after a
COPYFREEZE.
 
With this patch, the first and usually the last transforming query is performed much faster after initial load.

I have read this patch and have no outstanding comments on it.
Pavan Deolasee demonstrates the expected speed improvement.

The new status of this patch is: Ready for Committer

От:
Masahiko Sawada
Дата:

On Thu, Mar 21, 2019 at 11:27 PM Pavan Deolasee
<> wrote:
>
>
>
> On Thu, Mar 14, 2019 at 3:54 PM Masahiko Sawada <> wrote:
>>
>>
>> >
>> >
>> > Ok. I will run some tests. But please note that this patch is a bug fix to address the performance issue that is
causedby having to rewrite the entire table when all-visible bit is set on the page during first vacuum. So while we
maydo some more work during COPY FREEZE, we're saving a lot of page writes during next vacuum. Also, since the scan
thatwe are doing in this patch is done on a page that should be in the buffer cache, we will pay a bit in terms of CPU
cost,but not anything in terms of IO cost. 
>>
>> Agreed. I had been misunderstanding this patch. The page scan during
>> COPY FREEZE is necessary and it's very cheaper than doing in the first
>> vacuum.
>
>
> Thanks for agreeing to the need of this bug fix. I ran some simple tests anyways and here are the results.
>
> The test consists of a simple table with three columns, two integers and one char(100). I then ran COPY (FREEZE),
loading7M rows, followed by a VACUUM. The total size of the raw data is about 800MB and the table size in Postgres is
justunder 1GB. The results for 3 runs in milliseconds are: 
>
> Master:
>  COPY FREEZE: 40243.725   40309.675    40783.836
>  VACUUM: 2685.871  2517.445    2508.452
>
> Patched:
>  COPY FREEZE: 40942.410  40495.303   40638.075
>  VACUUM: 25.067  35.793   25.390
>
> So there is a slight increase in the time to run COPY FREEZE, but a significant reduction in time to VACUUM the
table.The benefits will only go up if the table is vacuumed much  later when most of the pages are already written to
thedisk and removed from shared buffers and/or kernel cache. 
>
> I hope this satisfies your doubts regarding performance implications of the patch.

Thank you for the performance testing, that's a great improvement!

I've looked at the patch and have comments and questions.

+    /*
+     * While we are holding the lock on the page, check if all tuples
+     * in the page are marked frozen at insertion. We can safely mark
+     * such page all-visible and set visibility map bits too.
+     */
+    if (CheckPageIsAllFrozen(relation, buffer))
+        PageSetAllVisible(page);
+
+    MarkBufferDirty(buffer);

Maybe we don't need to mark the buffer dirty if the page is not set all-visible.

-----
+    if (PageIsAllVisible(page))
+    {
+        uint8       vm_status = visibilitymap_get_status(relation,
+                targetBlock, &myvmbuffer);
+        uint8       flags = 0;
+
+        /* Set the VM all-frozen bit to flag, if needed */
+        if ((vm_status & VISIBILITYMAP_ALL_VISIBLE) == 0)
+            flags |= VISIBILITYMAP_ALL_VISIBLE;
+        if ((vm_status & VISIBILITYMAP_ALL_FROZEN) == 0)
+            flags |= VISIBILITYMAP_ALL_FROZEN;
+
+        Assert(BufferIsValid(myvmbuffer));
+        if (flags != 0)
+            visibilitymap_set(relation, targetBlock, buffer, InvalidXLogRecPtr,
+                    myvmbuffer, InvalidTransactionId, flags);

Since CheckPageIsAllFrozen() is used only when inserting frozen tuples
CheckAndSetPageAllVisible() seems to be implemented for the same
situation. If we have CheckAndSetPageAllVisible() for only this
situation we would rather need to check that the VM status of the page
should be 0 and then set two flags to the page? The 'flags' will
always be (VISIBILITYMAP_ALL_FROZEN | VISIBILITYMAP_ALL_VISIBLE) in
copy freeze case. I'm confused that this function has both code that
assumes some special situations and code that can be used in generic
situations.

-----
Perhaps we can add some tests for this feature to pg_visibility module.


Regards,

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


От:
Pavan Deolasee
Дата:


On Fri, Mar 22, 2019 at 12:19 PM Masahiko Sawada <> wrote:
I've looked at the patch and have comments and questions.

+    /*
+     * While we are holding the lock on the page, check if all tuples
+     * in the page are marked frozen at insertion. We can safely mark
+     * such page all-visible and set visibility map bits too.
+     */
+    if (CheckPageIsAllFrozen(relation, buffer))
+        PageSetAllVisible(page);
+
+    MarkBufferDirty(buffer);

Maybe we don't need to mark the buffer dirty if the page is not set all-visible.


Yeah, makes sense. Fixed.
 
 If we have CheckAndSetPageAllVisible() for only this
situation we would rather need to check that the VM status of the page
should be 0 and then set two flags to the page? The 'flags' will
always be (VISIBILITYMAP_ALL_FROZEN | VISIBILITYMAP_ALL_VISIBLE) in
copy freeze case. I'm confused that this function has both code that
assumes some special situations and code that can be used in generic
situations.

If a second COPY FREEZE is run within the same transaction and if starts inserting into the page used by the previous COPY FREEZE, then the page will already be marked all-visible/all-frozen. So we can skip repeating the operation again. While it's quite unlikely that someone will do that and I can't think of a situation where only one of those flags will be set, I don't see a harm in keeping the code as is. This code is borrowed from vacuumlazy.c and at some point we can even move it to some common location.


Perhaps we can add some tests for this feature to pg_visibility module.


That's a good idea. Please see if the tests included in the attached patch are enough.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Pavan Deolasee
Дата:



On Wed, Mar 27, 2019 at 9:47 AM Masahiko Sawada <> wrote:

The patch looks good to me. There is no comment from me.


Thanks for your review! Updated patch attached since patch failed to apply after recent changes in the master.

Thanks,
Pavan
 
--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Tomas Vondra
Дата:

Hi,

I've been looking at this patch for a while, and it seems pretty much RFC,
so barring objections I'll take care of that once I do a bit more testing
and review. Unless someone else wants to take care of that.

FWIW I wonder if we should add the code for partitioned tables to
CopyFrom, considering that's unsupported and so can't be tested etc. It's
not a huge amount of code, of course.

regards

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




От:
Andres Freund
Дата:

Hi,

On 2019-04-03 10:19:17 +0530, Pavan Deolasee wrote:
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index c1fd7b78ce..09df70a3ac 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -2833,6 +2833,15 @@ CopyFrom(CopyState cstate)
>                      !has_instead_insert_row_trig &&
>                      resultRelInfo->ri_FdwRoutine == NULL;
>  
> +                /*
> +                 * Note: As of PG12, COPY FREEZE is not supported on
> +                 * partitioned table. Nevertheless have this check in place so
> +                 * that we do the right thing if it ever gets supported.
> +                 */
> +                if (ti_options & TABLE_INSERT_FROZEN)
> +                    CheckAndSetAllVisibleBulkInsertState(resultRelInfo->ri_RelationDesc,
> +                            bistate);
> +
>                  /*
>                   * We'd better make the bulk insert mechanism gets a new
>                   * buffer when the partition being inserted into changes.
> @@ -3062,6 +3071,15 @@ CopyFrom(CopyState cstate)
>                                  firstBufferedLineNo);
>      }
>  
> +    /*
> +     * If we are inserting frozen tuples, check if the last page used can also
> +     * be marked as all-visible and all-frozen. This ensures that a table can
> +     * be fully frozen when the data is loaded.
> +     */
> +    if (ti_options & TABLE_INSERT_FROZEN)
> +        CheckAndSetAllVisibleBulkInsertState(resultRelInfo->ri_RelationDesc,
> +                bistate);
> +
>      /* Done, clean up */
>      error_context_stack = errcallback.previous;

I'm totally not OK with this from a layering
POV. CheckAndSetAllVisibleBulkInsertState is entirely heap specific
(without being named such), whereas all the heap specific bits are
getting moved below tableam.

Greetings,

Andres Freund



От:
Alvaro Herrera
Дата:

On 2019-Apr-04, Andres Freund wrote:

> I'm totally not OK with this from a layering
> POV. CheckAndSetAllVisibleBulkInsertState is entirely heap specific
> (without being named such), whereas all the heap specific bits are
> getting moved below tableam.

This is a fair complaint, but on the other hand the COPY changes for
table AM are still being developed, so there's no ground on which to
rebase this patch.  Do you have a timeline on getting the COPY one
committed?

I think it's fair to ask the RMT for an exceptional extension of a
couple of working days for this patch.

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



От:
Andres Freund
Дата:

Hi,

On 2019-04-04 16:15:54 -0300, Alvaro Herrera wrote:
> On 2019-Apr-04, Andres Freund wrote:
> 
> > I'm totally not OK with this from a layering
> > POV. CheckAndSetAllVisibleBulkInsertState is entirely heap specific
> > (without being named such), whereas all the heap specific bits are
> > getting moved below tableam.
> 
> This is a fair complaint, but on the other hand the COPY changes for
> table AM are still being developed, so there's no ground on which to
> rebase this patch.  Do you have a timeline on getting the COPY one
> committed?

~2h. Just pondering the naming of some functions etc.  Don't think
there's a large interdependency though.

But even if tableam weren't committed, I'd still argue that it's
structurally done wrong in the patch right now.  FWIW, I actually think
this whole approach isn't quite right - this shouldn't be done as a
secondary action after we'd already inserted, with a separate
lock-unlock cycle etc.

Also, how is this code even close to correct?
CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and
there's no WAL logging? Without even a comment arguing why that's OK (I
don't think it is)?

Greetings,

Andres Freund



От:
Andres Freund
Дата:

Hi,

On 2019-04-04 12:23:08 -0700, Andres Freund wrote:
> Also, how is this code even close to correct?
> CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and
> there's no WAL logging? Without even a comment arguing why that's OK (I
> don't think it is)?

Peter Geoghegan just reminded me over IM that there's actually logging
inside log_heap_visible(), called from visibilitymap_set(). Still lacks
a critical section though.

I still think this is the wrong architecture.

Greetings,

Andres Freund

PS: We're going to have to revamp visibilitymap_set() soon-ish - the
fact that it directly calls heap routines inside is bad, means that
additional AMs e.g. zheap has to reimplement that routine.



От:
Alvaro Herrera
Дата:

On 2019-Apr-04, Andres Freund wrote:

> On 2019-04-04 12:23:08 -0700, Andres Freund wrote:
> > Also, how is this code even close to correct?
> > CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and
> > there's no WAL logging? Without even a comment arguing why that's OK (I
> > don't think it is)?
> 
> Peter Geoghegan just reminded me over IM that there's actually logging
> inside log_heap_visible(), called from visibilitymap_set(). Still lacks
> a critical section though.

Hmm, isn't there already a critical section in visibilitymap_set itself?

> I still think this is the wrong architecture.

Hmm.

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



От:
Andres Freund
Дата:

Hi,

On 2019-04-05 00:06:04 -0300, Alvaro Herrera wrote:
> On 2019-Apr-04, Andres Freund wrote:
> 
> > On 2019-04-04 12:23:08 -0700, Andres Freund wrote:
> > > Also, how is this code even close to correct?
> > > CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and
> > > there's no WAL logging? Without even a comment arguing why that's OK (I
> > > don't think it is)?
> > 
> > Peter Geoghegan just reminded me over IM that there's actually logging
> > inside log_heap_visible(), called from visibilitymap_set(). Still lacks
> > a critical section though.
> 
> Hmm, isn't there already a critical section in visibilitymap_set itself?

There is, but the proposed code sets all visible on the page, and marks
the buffer dirty, before calling visibilitymap_set.

Greetings,

Andres Freund



От:
Andres Freund
Дата:

Hi,

On 2019-04-05 00:06:04 -0300, Alvaro Herrera wrote:
> On 2019-Apr-04, Andres Freund wrote:
> > I still think this is the wrong architecture.
> 
> Hmm.

I think the right approach would be to do all of this in heap_insert and
heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
is specified, remember whether it is either currently empty, or is
already marked as all-visible. If previously empty, mark it as all
visible at the end. If already all visible, there's no need to change
that. If not yet all-visible, no need to do anything, since it can't
have been inserted with COPY FREEZE.  Do you see any problem doing it
that way?

Greetings,

Andres Freund



От:
Pavan Deolasee
Дата:

Hi,

On Fri, Apr 5, 2019 at 9:05 AM Andres Freund <> wrote:


I think the right approach would be to do all of this in heap_insert and
heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
is specified, remember whether it is either currently empty, or is
already marked as all-visible. If previously empty, mark it as all
visible at the end. If already all visible, there's no need to change
that. If not yet all-visible, no need to do anything, since it can't
have been inserted with COPY FREEZE. 

We're doing roughly the same. If we are running INSERT_FROZEN, whenever we're about to switch to a new page, we check if the previous page should be marked all-frozen and do it that way. The special code in copy.c is necessary to take care of the last page which we don't get to handle in the regular code path.

Or are you suggesting that we don't even rescan the page for all-frozen tuples at the end and just simply mark it all-frozen at the start, when the first tuple is inserted and then don't touch the PD_ALL_VISIBLE/visibility map bit as we go on inserting more tuples in the page?

Anyways, if major architectural changes are required then it's probably too late to consider this for PG12, even though it's more of a bug fix and a candidate for back-patching too.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Pavan Deolasee
Дата:



On Fri, Apr 5, 2019 at 8:37 AM Andres Freund <> wrote:
Hi,

On 2019-04-05 00:06:04 -0300, Alvaro Herrera wrote:

>
> Hmm, isn't there already a critical section in visibilitymap_set itself?

There is, but the proposed code sets all visible on the page, and marks
the buffer dirty, before calling visibilitymap_set.

How's it any different than what we're doing at vacuumlazy.c:1322? We set the page-level bit, mark the buffer dirty and then call visibilitymap_set(), all outside a critical section.

1300         /* mark page all-visible, if appropriate */
1301         if (all_visible && !all_visible_according_to_vm)
1302         {
1303             uint8       flags = VISIBILITYMAP_ALL_VISIBLE;
1304 
1305             if (all_frozen)
1306                 flags |= VISIBILITYMAP_ALL_FROZEN;
1307 
1308             /*
1309              * It should never be the case that the visibility map page is set
1310              * while the page-level bit is clear, but the reverse is allowed
1311              * (if checksums are not enabled).  Regardless, set the both bits
1312              * so that we get back in sync.
1313              *
1314              * NB: If the heap page is all-visible but the VM bit is not set,
1315              * we don't need to dirty the heap page.  However, if checksums
1316              * are enabled, we do need to make sure that the heap page is
1317              * dirtied before passing it to visibilitymap_set(), because it
1318              * may be logged.  Given that this situation should only happen in
1319              * rare cases after a crash, it is not worth optimizing.
1320              */
1321             PageSetAllVisible(page);
1322             MarkBufferDirty(buf);
1323             visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
1324                               vmbuffer, visibility_cutoff_xid, flags);
1325         } 

As the first para in that comment says, I thought it's ok for page-level bit to be set but the visibility bit to be clear, but not the vice versa. The proposed code does not introduce any  new behaviour AFAICS. But I might be missing something.

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Tom Lane
Дата:

Andres Freund <> writes:
> I think the right approach would be to do all of this in heap_insert and
> heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> is specified, remember whether it is either currently empty, or is
> already marked as all-visible. If previously empty, mark it as all
> visible at the end. If already all visible, there's no need to change
> that. If not yet all-visible, no need to do anything, since it can't
> have been inserted with COPY FREEZE.  Do you see any problem doing it
> that way?

Do we want to add overhead to these hot-spot routines for this purpose?

            regards, tom lane



От:
Andres Freund
Дата:

Hi,

On 2019-04-05 09:20:36 +0530, Pavan Deolasee wrote:
> On Fri, Apr 5, 2019 at 9:05 AM Andres Freund <> wrote:
> > I think the right approach would be to do all of this in heap_insert and
> > heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> > is specified, remember whether it is either currently empty, or is
> > already marked as all-visible. If previously empty, mark it as all
> > visible at the end. If already all visible, there's no need to change
> > that. If not yet all-visible, no need to do anything, since it can't
> > have been inserted with COPY FREEZE.
> 
> 
> We're doing roughly the same. If we are running INSERT_FROZEN, whenever
> we're about to switch to a new page, we check if the previous page should
> be marked all-frozen and do it that way. The special code in copy.c is
> necessary to take care of the last page which we don't get to handle in the
> regular code path.

Well, it's not the same, because you need extra code from copy.c, extra
lock cycles, and extra WAL logging.


> Or are you suggesting that we don't even rescan the page for all-frozen
> tuples at the end and just simply mark it all-frozen at the start, when the
> first tuple is inserted and then don't touch the PD_ALL_VISIBLE/visibility
> map bit as we go on inserting more tuples in the page?

Correct. If done right that should be cheaper (no extra scans, less WAL
logging), without requiring some new dispatch logic from copy.c.


> Anyways, if major architectural changes are required then it's probably too
> late to consider this for PG12, even though it's more of a bug fix and a
> candidate for back-patching too.

Let's just see how bad it looks? I don't feel like we'd need to be super
strict about it. If it looks simple enough I'd e.g. be ok to merge this
soon after freeze, and backpatch around maybe 12.1 or such.

Greetings,

Andres Freund



От:
Andres Freund
Дата:

Hi,

On 2019-04-04 23:57:58 -0400, Tom Lane wrote:
> Andres Freund <> writes:
> > I think the right approach would be to do all of this in heap_insert and
> > heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> > is specified, remember whether it is either currently empty, or is
> > already marked as all-visible. If previously empty, mark it as all
> > visible at the end. If already all visible, there's no need to change
> > that. If not yet all-visible, no need to do anything, since it can't
> > have been inserted with COPY FREEZE.  Do you see any problem doing it
> > that way?
> 
> Do we want to add overhead to these hot-spot routines for this purpose?

For heap_multi_insert I can't see it being a problem - it's only used
from copy.c, and the cost should be "smeared" over many tuples.  I'd
assume that compared with locking a page, WAL logging, etc, it'd not
even meaningfully show up for heap_insert. Especially because we already
have codepaths for options & HEAP_INSERT_FROZEN in
heap_prepare_insert(), and I'd assume those could be combined.

I think we should measure it, but I don't think that one or two
additional, very well predictd, branches are going to be measurable in
in those routines.

The patch, as implemented, has modifications in
RelationGetBufferForTuple(), that seems like they'd be more expensive.

Greetings,

Andres Freund



От:
Darafei "Komяpa" Praliaskouski
Дата:



On Fri, Apr 5, 2019 at 6:58 AM Tom Lane <> wrote:
Andres Freund <> writes:
> I think the right approach would be to do all of this in heap_insert and
> heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> is specified, remember whether it is either currently empty, or is
> already marked as all-visible. If previously empty, mark it as all
> visible at the end. If already all visible, there's no need to change
> that. If not yet all-visible, no need to do anything, since it can't
> have been inserted with COPY FREEZE.  Do you see any problem doing it
> that way?

Do we want to add overhead to these hot-spot routines for this purpose?

Sizing the overhead: workflows right now don't end with COPY FREEZE - you need another VACUUM to set maps. 
Anything that lets you skip that VACUUM (and faster than that VACUUM itself) is helping. You specifically asked for it to be skippable with FREEZE anyway.


--
Darafei Praliaskouski
От:
Andres Freund
Дата:

Hi,

On 2019-04-05 08:38:34 +0300, Darafei "Komяpa" Praliaskouski wrote:
> On Fri, Apr 5, 2019 at 6:58 AM Tom Lane <> wrote:
> 
> > Andres Freund <> writes:
> > > I think the right approach would be to do all of this in heap_insert and
> > > heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> > > is specified, remember whether it is either currently empty, or is
> > > already marked as all-visible. If previously empty, mark it as all
> > > visible at the end. If already all visible, there's no need to change
> > > that. If not yet all-visible, no need to do anything, since it can't
> > > have been inserted with COPY FREEZE.  Do you see any problem doing it
> > > that way?
> >
> > Do we want to add overhead to these hot-spot routines for this purpose?
> >
> 
> Sizing the overhead: workflows right now don't end with COPY FREEZE - you
> need another VACUUM to set maps.
> Anything that lets you skip that VACUUM (and faster than that VACUUM
> itself) is helping. You specifically asked for it to be skippable with
> FREEZE anyway.

Tom's point was that the routines I was suggesting to adapt above aren't
just used for COPY FREEZE.

Greetings,

Andres Freund



От:
Andres Freund
Дата:

Hi,

On 2019-04-04 21:04:49 -0700, Andres Freund wrote:
> On 2019-04-04 23:57:58 -0400, Tom Lane wrote:
> > Andres Freund <> writes:
> > > I think the right approach would be to do all of this in heap_insert and
> > > heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> > > is specified, remember whether it is either currently empty, or is
> > > already marked as all-visible. If previously empty, mark it as all
> > > visible at the end. If already all visible, there's no need to change
> > > that. If not yet all-visible, no need to do anything, since it can't
> > > have been inserted with COPY FREEZE.  Do you see any problem doing it
> > > that way?
> > 
> > Do we want to add overhead to these hot-spot routines for this purpose?
> 
> For heap_multi_insert I can't see it being a problem - it's only used
> from copy.c, and the cost should be "smeared" over many tuples.  I'd
> assume that compared with locking a page, WAL logging, etc, it'd not
> even meaningfully show up for heap_insert. Especially because we already
> have codepaths for options & HEAP_INSERT_FROZEN in
> heap_prepare_insert(), and I'd assume those could be combined.
> 
> I think we should measure it, but I don't think that one or two
> additional, very well predictd, branches are going to be measurable in
> in those routines.
> 
> The patch, as implemented, has modifications in
> RelationGetBufferForTuple(), that seems like they'd be more expensive.

Here's a *prototype* patch for this.  It only implements what I
described for heap_multi_insert, not for plain inserts. I wanted to see
what others think before investing additional time.

I don't think it's possible to see the overhead of the changed code in
heap_multi_insert(), and probably - with less confidence - that it's
also going to be ok for heap_insert(). But we gotta measure that.


This avoids an extra WAL record for setting empty pages to all visible,
by adding XLH_INSERT_ALL_VISIBLE_SET & XLH_INSERT_ALL_FROZEN_SET, and
setting those when appropriate in heap_multi_insert.  Unfortunately
currently visibilitymap_set() doesn't really properly allow to do this,
as it has embedded WAL logging for heap.

I think we should remove the WAL logging from visibilitymap_set(), and
move it to a separate, heap specific, function. Right now different
tableams e.g. would have to reimplement visibilitymap_set(), so that's a
second need to separate that functionality.  Let me try to come up with
a proposal.


The patch currently does a vmbuffer_pin() while holding an exclusive
lwlock on the page. That's something we normally try to avoid - but I
think it's probably OK here, because INSERT_FROZEN can only be used to
insert into a new relfilenode (which thus no other session can see). I
think it's preferrable to have this logic in specific to the
INSERT_FROZEN path, rather than adding nontrivial complications to
RelationGetBufferForTuple().

I noticed that, before this patch, we do a
    if (vmbuffer != InvalidBuffer)
        ReleaseBuffer(vmbuffer);
after every filled page - that doesn't strike me as particularly smart -
it's pretty likely that the next heap page to be filled is going to be
on the same vm page as the previous iteration.


I noticed one small oddity that I think is common to all the approaches
presented in this thread so far. After

BEGIN;
TRUNCATE foo;
COPY foo(i) FROM '/tmp/foo' WITH FREEZE;
COPY foo(i) FROM '/tmp/foo' WITH FREEZE;
COPY foo(i) FROM '/tmp/foo' WITH FREEZE;
COMMIT;

we currently end up with pages like:
┌───────┬───────────┬──────────┬───────┬───────┬───────┬─────────┬──────────┬─────────┬───────────┐
│ blkno │    lsn    │ checksum │ flags │ lower │ upper │ special │ pagesize │ version │ prune_xid │
├───────┼───────────┼──────────┼───────┼───────┼───────┼─────────┼──────────┼─────────┼───────────┤
│     0 │ 0/50B5488 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     1 │ 0/50B6360 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     2 │ 0/50B71B8 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     3 │ 0/50B8028 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     4 │ 0/50B8660 │        0 │     4 │   408 │  5120 │    8192 │     8192 │       4 │         0 │
│     5 │ 0/50B94B8 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     6 │ 0/50BA328 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     7 │ 0/50BB180 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     8 │ 0/50BBFD8 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│     9 │ 0/50BCF88 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│    10 │ 0/50BDDE0 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│    11 │ 0/50BEC50 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│    12 │ 0/50BFAA8 │        0 │     4 │   928 │   960 │    8192 │     8192 │       4 │         0 │
│    13 │ 0/50C06F8 │        0 │     4 │   792 │  2048 │    8192 │     8192 │       4 │         0 │
└───────┴───────────┴──────────┴───────┴───────┴───────┴─────────┴──────────┴─────────┴───────────┘
(14 rows)

Note how block 4 has more space available. That's because the
visibilitymap_pin() called in the first COPY has to vm_extend(), which
in turn does:

    /*
     * Send a shared-inval message to force other backends to close any smgr
     * references they may have for this rel, which we are about to change.
     * This is a useful optimization because it means that backends don't have
     * to keep checking for creation or extension of the file, which happens
     * infrequently.
     */
    CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode);

which invalidates ->rd_smgr->smgr_targblock *after* the first COPY,
because that's when the pending smgr invalidations are sent out.  That's
far from great, but it doesn't seem to be this patch's fault.

It seems to me we need a separate invalidation that doesn't close the
whole smgr relation, but just invalidates the VM specific fields.

Greetings,

Andres Freund

От:
Andres Freund
Дата:

Hi,

On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
> Here's a *prototype* patch for this.  It only implements what I
> described for heap_multi_insert, not for plain inserts. I wanted to see
> what others think before investing additional time.

Pavan, are you planning to work on this for v13 CF1? Or have you lost
interest on the topic?

- Andres



От:
Pavan Deolasee
Дата:



On Tue, 28 May 2019 at 4:36 PM, Andres Freund <> wrote:
Hi,

On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
> Here's a *prototype* patch for this.  It only implements what I
> described for heap_multi_insert, not for plain inserts. I wanted to see
> what others think before investing additional time.

Pavan, are you planning to work on this for v13 CF1? Or have you lost
interest on the topic?

Yes, I plan to work on it.

Thanks,
Pavan
--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Pavan Deolasee
Дата:

Hi Andres,

On Wed, May 29, 2019 at 1:50 PM Pavan Deolasee <> wrote:


On Tue, 28 May 2019 at 4:36 PM, Andres Freund <> wrote:
Hi,

On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
> Here's a *prototype* patch for this.  It only implements what I
> described for heap_multi_insert, not for plain inserts. I wanted to see
> what others think before investing additional time.

Pavan, are you planning to work on this for v13 CF1? Or have you lost
interest on the topic?

Yes, I plan to work on it.


I am sorry, but I am not able to find time to get back to this because of other high priority items. If it still remains unaddressed in the next few weeks, I will pick it up again. But for now, I am happy if someone wants to pick and finish the work. 

Thanks,
Pavan

--
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
От:
Amit Kapila
Дата:

On Thu, Jun 27, 2019 at 11:02 AM Pavan Deolasee
<> wrote:
>
>>> On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
>>> > Here's a *prototype* patch for this.  It only implements what I
>>> > described for heap_multi_insert, not for plain inserts. I wanted to see
>>> > what others think before investing additional time.
>>>
>>> Pavan, are you planning to work on this for v13 CF1? Or have you lost
>>> interest on the topic?
>>
>>
>> Yes, I plan to work on it.
>>
>
> I am sorry, but I am not able to find time to get back to this because of other high priority items. If it still
remainsunaddressed in the next few weeks, I will pick it up again. But for now, I am happy if someone wants to pick and
finishthe work.
 
>

Fair enough, I have marked the entry [1] in the coming CF as "Returned
with Feedback".  I hope that is okay with you.

[1] - https://commitfest.postgresql.org/23/2009/

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