Hi,
I've pushed the fix, after a a couple extra rounds of careful testing.
I noticed that the existing pg_visibility regression tests don't check
if we freeze the TOAST rows too (failing to do that was one of the
symptoms). It'd be good to add that, because that would fail even
without CLOBBE
On 1/23/21 1:10 AM, Tom Lane wrote:
Pavan Deolasee writes:
On Mon, Jan 18, 2021 at 3:02 AM Tomas Vondra
wrote:
Pushed. Thanks everyone for the effort put into this patch. The first
version was sent in 2015, so it took quite a bit of time.
Thanks Tomas, Anastasia and everyone else who wo
Pavan Deolasee writes:
> On Mon, Jan 18, 2021 at 3:02 AM Tomas Vondra
> wrote:
>> Pushed. Thanks everyone for the effort put into this patch. The first
>> version was sent in 2015, so it took quite a bit of time.
> Thanks Tomas, Anastasia and everyone else who worked on the patch and
> ensured t
On Mon, Jan 18, 2021 at 3:02 AM Tomas Vondra
wrote:
>
>
> Pushed. Thanks everyone for the effort put into this patch. The first
> version was sent in 2015, so it took quite a bit of time.
>
>
Thanks Tomas, Anastasia and everyone else who worked on the patch and
ensured that it gets into the tree.
> Pushed. Thanks everyone for the effort put into this patch. The first
> version was sent in 2015, so it took quite a bit of time.
Great news. Thanks everyone who have been working on this.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:h
On 1/16/21 11:18 PM, Tomas Vondra wrote:
...
>
Thanks for the updated patch, this version looks OK to me - I've marked
it as RFC. I'll do a bit more testing, review, and then I'll get it
committed.
Pushed. Thanks everyone for the effort put into this patch. The first
version was sent in 2
On 1/16/21 4:11 PM, Anastasia Lubennikova wrote:
> ...
As Pavan correctly figured it out before the problem is that
RelationGetBufferForTuple() moves to the next page, losing free space in
the block:
> ... I see that a relcache invalidation arrives
> after 1st and then after every 32672
On 12.01.2021 22:30, Tomas Vondra wrote:
Thanks. These patches seem to resolve the TOAST table issue, freezing
it as expected. I think the code duplication is not an issue, but I
wonder why heap_insert uses this condition:
/*
* ...
*
* No need to update the visibilitymap if
Thanks. These patches seem to resolve the TOAST table issue, freezing it
as expected. I think the code duplication is not an issue, but I wonder
why heap_insert uses this condition:
/*
* ...
*
* No need to update the visibilitymap if it had all_frozen bit set
* before th
On 12.01.2021 00:51, Tomas Vondra wrote:
On 1/11/21 10:00 PM, Anastasia Lubennikova wrote:
On 11.01.2021 01:35, Tomas Vondra wrote:
Hi,
I started looking at this patch again, hoping to get it committed in
this CF, but I think there's a regression in handling TOAST tables
(compared to the v
On 1/11/21 10:00 PM, Anastasia Lubennikova wrote:
On 11.01.2021 01:35, Tomas Vondra wrote:
Hi,
I started looking at this patch again, hoping to get it committed in
this CF, but I think there's a regression in handling TOAST tables
(compared to the v3 patch submitted by Pavan in February 20
On 11.01.2021 01:35, Tomas Vondra wrote:
Hi,
I started looking at this patch again, hoping to get it committed in
this CF, but I think there's a regression in handling TOAST tables
(compared to the v3 patch submitted by Pavan in February 2019).
The test I'm running a very simple test (see te
Hi,
I started looking at this patch again, hoping to get it committed in
this CF, but I think there's a regression in handling TOAST tables
(compared to the v3 patch submitted by Pavan in February 2019).
The test I'm running a very simple test (see test.sql):
1) start a transaction
2) create
On Mon, Nov 02, 2020 at 04:44:22PM +0300, Anastasia Lubennikova wrote:
On 30.10.2020 03:42, Tomas Vondra wrote:
Hi,
I might be somewhat late to the party, but I've done a bit of
benchmarking too ;-) I used TPC-H data from a 100GB test, and tried
different combinations of COPY [FREEZE] and VACUU
On 30.10.2020 03:42, Tomas Vondra wrote:
Hi,
I might be somewhat late to the party, but I've done a bit of
benchmarking too ;-) I used TPC-H data from a 100GB test, and tried
different combinations of COPY [FREEZE] and VACUUM [FREEZE], both on
current master and with the patch.
So I don't reall
Hi,
I might be somewhat late to the party, but I've done a bit of
benchmarking too ;-) I used TPC-H data from a 100GB test, and tried
different combinations of COPY [FREEZE] and VACUUM [FREEZE], both on
current master and with the patch.
The results look like this (the columns say what combinati
> Status update for a commitfest entry.
>
> This patch is ReadyForCommitter. It applies and passes the CI. There are no
> unanswered questions in the discussion.
>
> The discussion started in 2015 with a patch by Jeff Janes. Later it was
> revived by Pavan Deolasee. After it was picked up by
Status update for a commitfest entry.
This patch is ReadyForCommitter. It applies and passes the CI. There are no
unanswered questions in the discussion.
The discussion started in 2015 with a patch by Jeff Janes. Later it was revived
by Pavan Deolasee. After it was picked up by Ibrar Ahmed an
On Thu, Aug 27, 2020 at 2:14 AM Anastasia Lubennikova <
a.lubennik...@postgrespro.ru> wrote:
> On 21.08.2020 19:43, Ibrar Ahmed wrote:
>
>
>
> On Wed, Aug 19, 2020 at 6:15 PM Anastasia Lubennikova <
> a.lubennik...@postgrespro.ru> wrote:
>
>> On 18.08.2020 02:54, Alvaro Herrera wrote:
>> > On 2020
On 21.08.2020 19:43, Ibrar Ahmed wrote:
On Wed, Aug 19, 2020 at 6:15 PM Anastasia Lubennikova
mailto:a.lubennik...@postgrespro.ru>>
wrote:
On 18.08.2020 02:54, Alvaro Herrera wrote:
> On 2020-Aug-14, Ibrar Ahmed wrote:
>
>> The table used for the test contains three columns
On Wed, Aug 19, 2020 at 6:15 PM Anastasia Lubennikova <
a.lubennik...@postgrespro.ru> wrote:
> On 18.08.2020 02:54, Alvaro Herrera wrote:
> > On 2020-Aug-14, Ibrar Ahmed wrote:
> >
> >> The table used for the test contains three columns (integer, text,
> >> varchar).
> >> The total number of rows
On 18.08.2020 02:54, Alvaro Herrera wrote:
On 2020-Aug-14, Ibrar Ahmed wrote:
The table used for the test contains three columns (integer, text,
varchar).
The total number of rows is 1000 in total.
Unpatched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
COPY: 9069.432 ms vacuum; 2567.
On 2020-Aug-14, Ibrar Ahmed wrote:
> The table used for the test contains three columns (integer, text,
> varchar).
> The total number of rows is 1000 in total.
>
> Unpatched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
> COPY: 9069.432 ms vacuum; 2567.961ms
> COPY: 9004.533 ms vacuum:
On Mon, Aug 17, 2020 at 2:19 PM Hamid Akhtar wrote:
> Unfortunately the latest patch doesn't apply cleanly on the master branch.
> Can you please share an updated one.
Please see the attached patch rebased with master (
a28d731a1187e8d9d8c2b6319375fcbf0a8debd5)
--
Ibrar Ahmed
copy-freeze-vm_
Unfortunately the latest patch doesn't apply cleanly on the master branch. Can
you please share an updated one.
On Mon, Aug 3, 2020 at 2:29 PM Anastasia Lubennikova <
a.lubennik...@postgrespro.ru> wrote:
> On 31.07.2020 23:28, Robert Haas wrote:
> > On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
> > wrote:
> >> Questions from the first review pass:
> >>
> >> 1) Do we need XLH_INSERT_ALL_VISIBLE_SET
On 31.07.2020 23:28, Robert Haas wrote:
On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
wrote:
Questions from the first review pass:
1) Do we need XLH_INSERT_ALL_VISIBLE_SET ? IIUC, in the patch it is always
implied by XLH_INSERT_ALL_FROZEN_SET.
I agree that it looks unnecessary to have
On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
wrote:
> Questions from the first review pass:
>
> 1) Do we need XLH_INSERT_ALL_VISIBLE_SET ? IIUC, in the patch it is always
> implied by XLH_INSERT_ALL_FROZEN_SET.
I agree that it looks unnecessary to have two separate bits.
> 2) What does
On 01.07.2020 12:38, Daniel Gustafsson wrote:
This patch incurs a compiler warning, which probably is quite simple to fix:
heapam.c: In function ‘heap_multi_insert’:
heapam.c:2349:4: error: ‘recptr’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
visibilitymap_set(
This patch incurs a compiler warning, which probably is quite simple to fix:
heapam.c: In function ‘heap_multi_insert’:
heapam.c:2349:4: error: ‘recptr’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
On Tue, Mar 24, 2020 at 10:06 PM Ibrar Ahmed wrote:
>
>
> On Fri, Mar 13, 2020 at 6:58 AM Justin Pryzby
> wrote:
>
>>
>> Thanks for picking up this patch. There's a minor typo:
>>
>> +* readable outside of this sessoin. Therefore
>> doing IO here isn't
>>
>> => session
>
On Fri, Mar 13, 2020 at 6:58 AM Justin Pryzby wrote:
>
> Thanks for picking up this patch. There's a minor typo:
>
> +* readable outside of this sessoin. Therefore
> doing IO here isn't
>
> => session
>
> --
> Justin
>
Thanks, please see the updated and rebased patch. (m
On Sat, Jun 29, 2019 at 12:56 AM Amit Kapila
wrote:
> 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 pla
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 investi
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
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
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
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 s
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 IN
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
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 i
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 whethe
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, mar
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, befo
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
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_FRO
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
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'
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 remin
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
> > gett
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
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)
>
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, conside
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.2
Thank you for sharing the updated patch!
On Tue, Mar 26, 2019 at 6:26 PM Pavan Deolasee wrote:
>
>
> 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, chec
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
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 caused by having to rewrite the
>> > e
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 PostGI
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
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 caused by having to rewrite the entire
table when all-visible bit is se
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 CO
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
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
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 i
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 had also posted a draft patch.
>
> I now have what I think is a more complete patch. I took a slightly different
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
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
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 beca
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
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))
== Fi
70 matches
Mail list logo
