Hi Kuntal,
On Thu, Jul 25, 2019 at 5:40 PM Kuntal Ghosh wrote:
> Here are some review comments on 0003-Add-undo-log-manager.patch. I've
> tried to avoid duplicate comments as much as possible.
Thanks! Replies inline. I'll be posting a new patch set shortly with
these and other fixes.
> 1. In
Hi Amit,
I've combined three of your messages into one below, and responded
inline. New patch set to follow shortly, with the fixes listed below
(and others from other reviewers).
On Wed, Jul 24, 2019 at 9:15 PM Amit Kapila wrote:
> 0003-Add-undo-log-manager.patch
> 1.
>
On Wed, Aug 14, 2019 at 2:48 PM Dilip Kumar wrote:
>
> On Wed, Aug 14, 2019 at 12:27 PM Andres Freund wrote:
> > I think that batch reading should just copy the underlying data into a
> > char* buffer. Only the records that currently are being used by
> > higher layers should get exploded
On Wed, Aug 14, 2019 at 10:35 PM Andres Freund wrote:
>
> Hi,
>
> On 2019-08-14 14:48:07 +0530, Dilip Kumar wrote:
> > On Wed, Aug 14, 2019 at 12:27 PM Andres Freund wrote:
> > > - I think there's two fairly fundamental, and related, problems with
> > > the sequence outlined above:
> > >
> > >
Hi,
On 2019-08-14 14:48:07 +0530, Dilip Kumar wrote:
> On Wed, Aug 14, 2019 at 12:27 PM Andres Freund wrote:
> > - I think there's two fairly fundamental, and related, problems with
> > the sequence outlined above:
> >
> > - We can't search for target buffers to store undo data, while
Hi,
On 2019-08-13 17:05:27 +0530, Dilip Kumar wrote:
> On Mon, Aug 5, 2019 at 11:59 PM Andres Freund wrote:
> > (as I was out of context due to dealing with bugs, I've switched to
> > lookign at the current zheap/undoprocessing branch.
> >
> > On 2019-07-30 01:02:20 -0700, Andres Freund wrote:
>
Hi,
On 2019-08-13 13:53:59 +0530, Dilip Kumar wrote:
> On Tue, Jul 30, 2019 at 1:32 PM Andres Freund wrote:
> > > + /* Loop until we have fetched all the buffers in which we need to
> > > write. */
> > > + while (size > 0)
> > > + {
> > > + bufidx =
On Wed, Aug 14, 2019 at 12:27 PM Andres Freund wrote:
>
> Hi,
>
> - I think there's two fairly fundamental, and related, problems with
> the sequence outlined above:
>
> - We can't search for target buffers to store undo data, while holding
> the "table" page content locked.
>
> The
Hi,
On 2019-08-06 14:18:42 -0700, Andres Freund wrote:
> Here's the last section of my low-leve review. Plan to write a higher
> level summary afterwards, now that I have a better picture of the code.
General comments:
- For a feature of this complexity, there's very little architectural
On Tue, Aug 13, 2019 at 6:28 AM Amit Kapila wrote:
> So, for top-level transactions rollback, we can directly refer from
> UndoRequest *, the start and end locations. But, what should we do
> for sub-transactions (rollback to savepoint)? One related point is
> that we also need information
On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
>
> Hi,
>
> On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> > Need to do something else for a bit. More later.
>
>
> > + /*
> > + * Compute the header size of the undo record.
> > + */
> > +Size
> > +UndoRecordHeaderSize(uint16 uur_info)
On Mon, Aug 5, 2019 at 11:59 PM Andres Freund wrote:
>
> Hi,
>
> (as I was out of context due to dealing with bugs, I've switched to
> lookign at the current zheap/undoprocessing branch.
>
> On 2019-07-30 01:02:20 -0700, Andres Freund wrote:
> > +/*
> > + * Insert a previously-prepared undo
On Thu, Aug 8, 2019 at 7:01 PM Andres Freund wrote:
> On 2019-08-07 14:50:17 +0530, Amit Kapila wrote:
> > On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
> > > On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
>
> > > > typedef struct TwoPhaseFileHeader
> > > > {
> > > > @@ -927,6
On Fri, Aug 9, 2019 at 1:57 AM Robert Haas wrote:
>
> On Thu, Aug 8, 2019 at 9:31 AM Andres Freund wrote:
> > I know that Robert is working on a patch that revises the undo request
> > layer somewhat, it's possible that this is best discussed afterwards.
>
> Here's what I have at the moment.
On Tue, Jul 30, 2019 at 1:32 PM Andres Freund wrote:
>
> Hi,
>
> Amit, short note: The patches aren't attached in patch order. Obviously
> a miniscule thing, but still nicer if that's not the case.
>
> Dilip, this also contains the start of a review for the undo record
> interface further down.
On Tue, Jul 30, 2019 at 12:21 PM Thomas Munro wrote:
>
> Hi Dilip,
>
> > commit 2f3c127b9e8bc7d27cf7adebff0a355684dfb94e
> > Author: Dilip Kumar
> > Date: Thu May 2 11:28:13 2019 +0530
> >
> >Provide interfaces to store and fetch undo records.
>
> +#include "commands/tablecmds.h"
>
On Wed, Jul 24, 2019 at 11:28 AM Rushabh Lathia
wrote:
>
> Hi,
>
> I have stated review of
> 0008-Provide-interfaces-to-store-and-fetch-undo-records.patch, here are few
> quick comments.
>
> 1) README.undointerface should provide more information like API details or
> the sequence in which API
On Fri, Jul 19, 2019 at 2:28 PM Amit Kapila wrote:
>
> On Thu, Jul 11, 2019 at 9:17 AM Dilip Kumar wrote:
> >
> > On Thu, Jul 11, 2019 at 12:38 AM Robert Haas wrote:
> > >
> > > I don't like the fact that undoaccess.c has a new global,
> > > undo_compression_info. I haven't read the code
On Thu, Jul 18, 2019 at 4:58 PM Amit Kapila wrote:
>
> On Tue, Jul 16, 2019 at 2:20 PM Dilip Kumar wrote:
> >
>
> Few comments on the new patch:
>
> 1.
> Additionally,
> +there is a mechanism for multi-insert, wherein multiple records are prepared
> +and inserted at a time.
>
> Which mechanism
On Wed, Aug 7, 2019 at 6:57 AM Heikki Linnakangas wrote:
> Yeah, that's also a problem with complicated WAL record types. Hopefully
> the complex cases are an exception, not the norm. A complex case is
> unlikely to fit any pre-defined set of fields anyway. (We could look at
> how e.g. protobuf
On Fri, Jul 26, 2019 at 9:57 PM Amit Khandekar wrote:
>
> On Fri, 26 Jul 2019 at 12:25, Amit Kapila wrote:
> > I agree with all your other comments.
>
> Thanks for addressing the comments. Below is the continuation of my
> comments from
On Tue, Jul 23, 2019 at 8:12 PM Amit Khandekar wrote:
>
>
>
> Some further review comments for undoworker.c :
>
>
> +/* Sets the worker's lingering status. */
> +static void
> +UndoWorkerIsLingering(bool sleep)
> The function name sounds like "is the worker lingering ?". Can
On Mon, Jul 22, 2019 at 8:39 PM Amit Khandekar wrote:
>
> On Mon, 22 Jul 2019 at 14:21, Amit Kapila wrote:
>
> I have started review of
> 0014-Allow-execution-and-discard-of-undo-by-background-wo.patch. Below
> are some quick comments to start with:
>
> +++ b/src/backend/access/undo/undoworker.c
On Fri, Aug 9, 2019 at 1:57 AM Robert Haas wrote:
>
> On Thu, Aug 8, 2019 at 9:31 AM Andres Freund wrote:
> > I know that Robert is working on a patch that revises the undo request
> > layer somewhat, it's possible that this is best discussed afterwards.
>
> Here's what I have at the moment.
On Thu, Aug 8, 2019 at 9:31 AM Andres Freund wrote:
> I know that Robert is working on a patch that revises the undo request
> layer somewhat, it's possible that this is best discussed afterwards.
Here's what I have at the moment. This is not by any means a complete
replacement for Amit's undo
Hi,
On 2019-08-07 14:50:17 +0530, Amit Kapila wrote:
> On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
> > On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> > > +/*
> > > + * Binary heap comparison function to compare the time at which an error
> > > + * occurred for transactions.
> > > +
On Wed, Aug 7, 2019 at 5:06 PM Thomas Munro wrote:
>
> On Thu, Aug 1, 2019 at 1:22 AM Amit Kapila wrote:
> > On Wed, Jul 31, 2019 at 10:13 AM Amit Kapila
> > wrote:
> > > On Tue, Jul 30, 2019 at 5:26 PM Thomas Munro
> > > wrote:
> > > > but
> > > > here's a small thing: I managed to reach
On Thu, Aug 1, 2019 at 1:22 AM Amit Kapila wrote:
> On Wed, Jul 31, 2019 at 10:13 AM Amit Kapila wrote:
> > On Tue, Jul 30, 2019 at 5:26 PM Thomas Munro wrote:
> > > but
> > > here's a small thing: I managed to reach an LWLock self-deadlock in
> > > the undo worker launcher:
> > >
> >
> > I
On 07/08/2019 13:52, Dilip Kumar wrote:
I have one more problem related to compression of the command id
field. Basically, the problem is that we don't set the command id in
the WAL and we will always store FirstCommandId in the undo[1]. So
suppose there were 2 operations under a different
On 05/08/2019 16:24, Robert Haas wrote:
On Sun, Aug 4, 2019 at 5:16 AM Heikki Linnakangas wrote:
I feel that the level of abstraction is not quite right. There are a
bunch of fields, like uur_block, uur_offset, uur_tuple, that are
probably useful for some UNDO resource managers (zheap I
On Tue, Jul 30, 2019 at 1:54 PM Dilip Kumar wrote:
>
> On Tue, Jul 30, 2019 at 12:21 PM Thomas Munro wrote:
> >
> > One data structure that could perhaps hold this would be
> > UndoLogTableEntry (the per-backend cache, indexed by undo log number,
> > with pretty fast lookups; used for things
On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
> On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
>
I am responding to some of the points where I need some more inputs or
some discussion is required. Some of the things need more thoughts
which I will respond later and some are quite
On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
>
> Hi,
>
> On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> > Need to do something else for a bit. More later.
>
> > + * false, otherwise.
> > + */
> > +static bool
> > +UndoAlreadyApplied(FullTransactionId full_xid, UndoRecPtr
On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
>
> Hi,
>
> On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> > Need to do something else for a bit. More later.
>
> Here we go.
>
Thanks for the review. I will work on them. Currently, I need
suggestions on some of the review comments.
>
>
Hello Andres,
On Tue, Aug 6, 2019 at 1:26 PM Andres Freund wrote:
> > +/* Each worker queue is a binary heap. */
> > +typedef struct
> > +{
> > + binaryheap *bh;
> > + union
> > + {
> > + UndoXidQueue *xid_elems;
> > + UndoSizeQueue *size_elems;
> > +
Hi,
I'll be responding to a bunch of long review emails in this thread
point by point separately, but just picking out a couple of points
here that jumped out at me:
On Wed, Aug 7, 2019 at 9:18 AM Andres Freund wrote:
> > + {
> > + /*
> > +
Hi,
On 2019-08-06 00:56:26 -0700, Andres Freund wrote:
> Out of energy.
Here's the last section of my low-leve review. Plan to write a higher
level summary afterwards, now that I have a better picture of the code.
> +static void
> +UndoDiscardOneLog(UndoLogSlot *slot, TransactionId xmin, bool
Hi,
On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> Need to do something else for a bit. More later.
Here we go.
> + /*
> + * Compute the header size of the undo record.
> + */
> +Size
> +UndoRecordHeaderSize(uint16 uur_info)
> +{
> + Sizesize;
> +
> + /* Add fixed
On Mon, Aug 5, 2019 at 6:29 PM Robert Haas wrote:
>
> On Mon, Aug 5, 2019 at 6:16 AM Amit Kapila wrote:
> > For zheap, we collect all the records of a page, apply them together
> > and then write the entire page in WAL. The progress of transaction is
> > updated at either transaction end
On Mon, Aug 5, 2019 at 12:42 PM Andres Freund wrote:
> A good move in the right direction, imo.
I spent some more time thinking about this and talking to Thomas about
it and I'd like to propose a somewhat more aggressive restructuring
proposal, with the aim of getting a cleaner separation
Hi,
(as I was out of context due to dealing with bugs, I've switched to
lookign at the current zheap/undoprocessing branch.
On 2019-07-30 01:02:20 -0700, Andres Freund wrote:
> +/*
> + * Insert a previously-prepared undo records.
> + *
> + * This function will write the actual undo record into
Hi,
On 2019-08-05 11:25:10 -0400, Robert Haas wrote:
> The obvious thing to do seems to be to have UndoLogControl objects own
> SmgrRelations. That would be something of a novelty, since it looks
> like currently only a Relation ever owns an SMgrRelation, but the smgr
> infrastructure seems to
On Tue, Jul 30, 2019 at 4:02 AM Andres Freund wrote:
> I'm a bit worried about expanding the use of
> ReadBufferWithoutRelcache(). Not so much because of the relcache itself,
> but because it requires doing separate smgropen() calls. While not
> crazily expensive, it's also not free. Especially
On Sun, Aug 4, 2019 at 5:16 AM Heikki Linnakangas wrote:
> I feel that the level of abstraction is not quite right. There are a
> bunch of fields, like uur_block, uur_offset, uur_tuple, that are
> probably useful for some UNDO resource managers (zheap I presume), but
> seem kind of arbitrary. How
On Mon, Aug 5, 2019 at 6:16 AM Amit Kapila wrote:
> For zheap, we collect all the records of a page, apply them together
> and then write the entire page in WAL. The progress of transaction is
> updated at either transaction end (rollback complete) or after
> processing some threshold of undo
On Mon, Aug 5, 2019 at 12:09 PM Heikki Linnakangas wrote:
>
> On 05/08/2019 07:23, Thomas Munro wrote:
> > On Mon, Aug 5, 2019 at 3:54 PM Amit Kapila wrote:
> >> On Sun, Aug 4, 2019 at 2:46 PM Heikki Linnakangas wrote:
> >>> Could we leave out the UNDO and discard worker processes for now?
>
On 05/08/2019 07:23, Thomas Munro wrote:
On Mon, Aug 5, 2019 at 3:54 PM Amit Kapila wrote:
On Sun, Aug 4, 2019 at 2:46 PM Heikki Linnakangas wrote:
Could we leave out the UNDO and discard worker processes for now?
Execute all UNDO actions immediately at rollback, and after crash
recovery.
On Mon, Aug 5, 2019 at 3:54 PM Amit Kapila wrote:
> On Sun, Aug 4, 2019 at 2:46 PM Heikki Linnakangas wrote:
> > I had a look at the UNDO patches at
> > https://www.postgresql.org/message-id/CAA4eK1KKAFBCJuPnFtgdc89djv4xO%3DZkAdXvKQinqN4hWiRbvA%40mail.gmail.com,
> > and at the patch to use the
On Sun, Aug 4, 2019 at 2:46 PM Heikki Linnakangas wrote:
>
> I had a look at the UNDO patches at
> https://www.postgresql.org/message-id/CAA4eK1KKAFBCJuPnFtgdc89djv4xO%3DZkAdXvKQinqN4hWiRbvA%40mail.gmail.com,
> and at the patch to use the UNDO logs to clean up orphaned files, from
>
I had a look at the UNDO patches at
https://www.postgresql.org/message-id/CAA4eK1KKAFBCJuPnFtgdc89djv4xO%3DZkAdXvKQinqN4hWiRbvA%40mail.gmail.com,
and at the patch to use the UNDO logs to clean up orphaned files, from
undo-2019-05-10.tgz earlier in this thread. Are these the latest ones to
On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar wrote:
>
> On Mon, Jul 22, 2019 at 2:21 PM Amit Kapila wrote:
> >
>
> I have reviewed 0012-Infrastructure-to-execute-pending-undo-actions,
> Please find my comment so far.
..
> 4.
> +void
> +undoaction_redo(XLogReaderState *record)
> +{
> + uint8 info
On Wed, Jul 31, 2019 at 10:13 AM Amit Kapila wrote:
>
> On Tue, Jul 30, 2019 at 5:26 PM Thomas Munro wrote:
> >
> > Hi Amit
> >
> > I've been testing some undo worker workloads (more on that soon),
> >
>
> One small point, there is one small bug in the error queues which is
> that the element
On Wed, Jul 24, 2019 at 10:00 AM Dilip Kumar wrote:
>
> On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar wrote:
> >
> Please find my review comments for
> 0013-Allow-foreground-transactions-to-perform-undo-action
>
> + /* initialize undo record locations for the transaction */
> + for (i = 0; i <
On Tue, Jul 30, 2019 at 5:26 PM Thomas Munro wrote:
>
> Hi Amit
>
> I've been testing some undo worker workloads (more on that soon),
>
One small point, there is one small bug in the error queues which is
that the element pushed into error queue doesn't have an updated value
of to_urec_ptr which
On Tue, Jul 30, 2019 at 1:32 PM Andres Freund wrote:
>
> Hi,
>
> Amit, short note: The patches aren't attached in patch order. Obviously
> a miniscule thing, but still nicer if that's not the case.
>
Noted, I will try to ensure that patches are in order in future posts.
--
With Regards,
Amit
Hi Amit
I've been testing some undo worker workloads (more on that soon), but
here's a small thing: I managed to reach an LWLock self-deadlock in
the undo worker launcher:
diff --git a/src/backend/access/undo/undorequest.c
b/src/backend/access/undo/undorequest.c
...
+bool
+UndoGetWork(bool
On Tue, Jul 30, 2019 at 12:21 PM Thomas Munro wrote:
>
> On Tue, Jul 30, 2019 at 5:03 PM Dilip Kumar wrote:
> >
> > I think this idea is good for the DO time but during REDO time it will
> > not work as we will not have the transaction state. Having said that
> > the current idea of keeping in
Hi,
Amit, short note: The patches aren't attached in patch order. Obviously
a miniscule thing, but still nicer if that's not the case.
Dilip, this also contains the start of a review for the undo record
interface further down.
On 2019-07-29 16:35:20 -0700, Andres Freund wrote:
>
Here we go.
On Tue, Jul 30, 2019 at 5:03 PM Dilip Kumar wrote:
> On Fri, Jul 19, 2019 at 2:28 PM Amit Kapila wrote:
> > On Thu, Jul 11, 2019 at 9:17 AM Dilip Kumar wrote:
> > > On Thu, Jul 11, 2019 at 12:38 AM Robert Haas
> > > wrote:
> > > > I don't like the fact that undoaccess.c has a new global,
> >
On Fri, Jul 19, 2019 at 2:28 PM Amit Kapila wrote:
>
> On Thu, Jul 11, 2019 at 9:17 AM Dilip Kumar wrote:
> >
> > On Thu, Jul 11, 2019 at 12:38 AM Robert Haas wrote:
> > >
> > > I don't like the fact that undoaccess.c has a new global,
> > > undo_compression_info. I haven't read the code
On Tue, Jul 30, 2019 at 12:18 AM Robert Haas wrote:
>
> On Tue, Jul 23, 2019 at 10:42 AM Amit Khandekar
> wrote:
>
> > Yes, I also think that the function would error out only because of
> > can't-happen cases, like "too many locks taken" or "out of binary heap
> > slots" or "out of memory"
Hi,
I realize that this might not be the absolutely newest version of the
undo storage part of this patchset - but I'm trying to understand the
whole context, and that's hard without reading through the whole stack
in a situation where the layers actually fit together
On 2019-07-29 01:48:30
On Tue, Jul 23, 2019 at 10:42 AM Amit Khandekar wrote:
> I think, even though there might not be a correctness issue with the
> current code as it stands, we should still use a local variable.
> Updating MyUndoWorker is a big side-effect, which the caller is not
> supposed to be aware of, because
On Sun, Jul 28, 2019 at 9:38 PM Thomas Munro wrote:
> Interesting. One way to bring back posix_fallocate() without
> upsetting people on some filesystem out there would be to turn the new
> wal_init_zero GUC into a choice: write (current default, and current
> behaviour for 'on'), pwrite_hole
Hi
On 2019-06-26 01:29:57 +0530, Amit Kapila wrote:
> From 67845a7afa675e973bd0ea9481072effa1eb219d Mon Sep 17 00:00:00 2001
> From: Dilip Kumar
> Date: Wed, 24 Apr 2019 14:36:28 +0530
> Subject: [PATCH 05/14] Add prefetch support for the undo log
>
> Add prefetching function for undo smgr and
On Sat, Jul 27, 2019 at 2:27 PM Andres Freund wrote:
> Note that neither of those mean that it's not a good idea to
> posix_fallocate() and *then* write zeroes, when initializing. For
> several filesystems that's more likely to result in more optimally sized
> filesystem extents, reducing
Hi,
On 2019-07-25 17:51:33 +1200, Thomas Munro wrote:
> 1. WAL's use of fdatasync(): The reason we fill and then fsync()
> newly created WAL files up front is because we want to make sure the
> blocks are definitely on disk. The comment doesn't spell out exactly
> why the author considered
On Fri, 26 Jul 2019 at 12:25, Amit Kapila wrote:
> I agree with all your other comments.
Thanks for addressing the comments. Below is the continuation of my
comments from 0014-Allow-execution-and-discard-of-undo-by-background-wo.patch
:
+ * Perform rollback request. We need to connect to the
On Wed, Jul 24, 2019 at 10:00 AM Dilip Kumar wrote:
>
> On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar wrote:
> >
> Please find my review comments for
> 0013-Allow-foreground-transactions-to-perform-undo-action
>
>
> + * We can't postpone applying undo actions for subtransactions as the
> + *
On Tue, Jul 23, 2019 at 8:12 PM Amit Khandekar wrote:
>
> On Tue, 23 Jul 2019 at 08:48, Amit Kapila wrote:
> >
> >
> > > --
> > >
> > > + if (!InsertRequestIntoErrorUndoQueue(urinfo))
> > > I was thinking what happens if for some reason
> > > InsertRequestIntoErrorUndoQueue() itself
On Thu, Jul 25, 2019 at 11:25 AM Dilip Kumar wrote:
>
> Hi Thomas,
>
> I have started reviewing 0003-Add-undo-log-manager, I haven't yet
> reviewed but some places I noticed that instead of UndoRecPtr you are
> directly
> using UndoLogOffset. Which seems like bugs to me
>
> 1.
> +UndoRecPtr
>
On Thu, Jul 25, 2019 at 11:22 AM Thomas Munro wrote:
>
> On Wed, Jul 24, 2019 at 9:15 PM Amit Kapila wrote:
> > I have done some more review of undolog patch series and here are my
> > comments:
>
> Hi Amit,
>
> Thanks! There a number of actionable changes in your review. I'll be
> posting a
Hi Thomas,
Few review comments on 0003-Add-undo-log-manager.patch:
1) Upgrade may fail
+/*
+ * Compute the new redo, and move the pg_undo file to match if necessary.
+ * Rather than renaming it, we'll create a new copy, so that a failure that
+ * occurs before the controlfile is rewritten won't
On Wed, Jul 24, 2019 at 2:45 PM Amit Kapila wrote:
>
> On Thu, Jul 18, 2019 at 5:10 PM Amit Kapila wrote:
> >
> > > Yep, that was completely wrong. Here's a new version.
> > >
> >
> > One comment/question related to
> > 0022-Use-undo-based-rollback-to-clean-up-files-on-abort.patch.
> >
>
> I
Hi Thomas,
I have started reviewing 0003-Add-undo-log-manager, I haven't yet
reviewed but some places I noticed that instead of UndoRecPtr you are
directly
using UndoLogOffset. Which seems like bugs to me
1.
+UndoRecPtr
+UndoLogAllocateInRecovery(UndoLogAllocContext *context,
+ TransactionId
On Wed, Jul 24, 2019 at 9:15 PM Amit Kapila wrote:
> I have done some more review of undolog patch series and here are my comments:
Hi Amit,
Thanks! There a number of actionable changes in your review. I'll be
posting a new patch set soon that will address most of your complaints
Hello Thomas,
Here are some review comments on 0003-Add-undo-log-manager.patch. I've
tried to avoid duplicate comments as much as possible.
1. In UndoLogAllocate,
+ * time this backend as needed to write to an undo log at all or because
s/as/has
+ * Maintain our tracking of the and the previous
On Thu, Jul 25, 2019 at 7:48 AM Amit Kapila wrote:
>
> On Wed, Jul 24, 2019 at 11:04 PM vignesh C wrote:
> >
> > Hi,
> >
> > I have done some review of undolog patch series
> > and here are my comments:
> > 0003-Add-undo-log-manager.patch
> >
> > 1) As undo log is being created in tablespace,
>
On Wed, Jul 24, 2019 at 11:04 PM vignesh C wrote:
>
> Hi,
>
> I have done some review of undolog patch series
> and here are my comments:
> 0003-Add-undo-log-manager.patch
>
> 1) As undo log is being created in tablespace,
> if the tablespace is dropped later, will it have any impact?
>
Yes, it
Hi,
I have done some review of undolog patch series
and here are my comments:
0003-Add-undo-log-manager.patch
1) As undo log is being created in tablespace,
if the tablespace is dropped later, will it have any impact?
+void
+UndoLogDirectory(Oid tablespace, char *dir)
+{
+ if (tablespace ==
On Wed, Jul 24, 2019 at 2:45 PM Amit Kapila wrote:
>
> On Thu, Jul 18, 2019 at 5:10 PM Amit Kapila wrote:
>
> 7.
> +attach_undo_log(UndoLogCategory category, Oid tablespace)
> {
> ..
> if (candidate->meta.tablespace == tablespace)
> + {
> + logno = *place;
> + slot = candidate;
> + *place =
On Wed, Jul 24, 2019 at 2:45 PM Amit Kapila wrote:
>
> On Thu, Jul 18, 2019 at 5:10 PM Amit Kapila wrote:
> >
> > On Mon, Jul 1, 2019 at 1:24 PM Thomas Munro wrote:
> > > Yep, that was completely wrong. Here's a new version.
>
> 10.
> I think UndoLogAllocate can leak allocation of slots. It
On Thu, Jul 18, 2019 at 5:10 PM Amit Kapila wrote:
>
> On Mon, Jul 1, 2019 at 1:24 PM Thomas Munro wrote:
> >
> > On Fri, Jun 28, 2019 at 6:09 AM Robert Haas wrote:
> > > I happened to open up 0001 from this series, which is from Thomas, and
> > > I do not think that the pg_buffercache changes
On Wed, Jul 24, 2019 at 11:28 AM Rushabh Lathia
wrote:
>
> Hi,
>
> I have stated review of
> 0008-Provide-interfaces-to-store-and-fetch-undo-records.patch, here are few
> quick comments.
Thanks for the review, I will work on them soon and post the updated
patch along with other comments. I have
On 2019-07-22 14:21:36 +0530, Amit Kapila wrote:
> Another thing is changing subxids to fxids can increase the size of
> two-phase file for a xact having many sub-transactions which again
> might be okay, but not completely sure.
I can't see that being a problem.
Hi,
I have stated review of
0008-Provide-interfaces-to-store-and-fetch-undo-records.patch, here are few
quick comments.
1) README.undointerface should provide more information like API details or
the sequence in which API should get called.
2) Information about the API's in the undoaccess.c
On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar wrote:
>
Please find my review comments for
0013-Allow-foreground-transactions-to-perform-undo-action
+ /* initialize undo record locations for the transaction */
+ for (i = 0; i < UndoLogCategories; i++)
+ {
+ s->start_urec_ptr[i] = InvalidUndoRecPtr;
On Tue, 23 Jul 2019 at 08:48, Amit Kapila wrote:
>
> On Mon, Jul 22, 2019 at 8:39 PM Amit Khandekar wrote:
> >
> > On Mon, 22 Jul 2019 at 14:21, Amit Kapila wrote:
> >
> > -
> >
> > +UndoWorkerGetSlotInfo(int slot, UndoRequestInfo *urinfo)
> > +{
> > + /* Block concurrent access. */
On Mon, Jul 22, 2019 at 8:39 PM Amit Khandekar wrote:
>
> On Mon, 22 Jul 2019 at 14:21, Amit Kapila wrote:
>
> -
>
> +UndoWorkerGetSlotInfo(int slot, UndoRequestInfo *urinfo)
> +{
> + /* Block concurrent access. */
> + LWLockAcquire(UndoWorkerLock, LW_EXCLUSIVE);
> +
> + MyUndoWorker
On Fri, 19 Jul 2019 at 17:24, Amit Khandekar wrote:
>
> On Thu, 9 May 2019 at 12:04, Dilip Kumar wrote:
>
> > Patches can be applied on top of undo branch [1] commit:
> > (cb777466d008e656f03771cf16ec7ef9d6f2778b)
> >
> > [1] https://github.com/EnterpriseDB/zheap/tree/undo
>
> Below are some
On Mon, 22 Jul 2019 at 14:21, Amit Kapila wrote:
I have started review of
0014-Allow-execution-and-discard-of-undo-by-background-wo.patch. Below
are some quick comments to start with:
+++ b/src/backend/access/undo/undoworker.c
+#include "access/xact.h"
+#include "access/undorequest.h"
Order is
On Mon, Jul 22, 2019 at 2:21 PM Amit Kapila wrote:
>
I have reviewed 0012-Infrastructure-to-execute-pending-undo-actions,
Please find my comment so far.
1.
+ /* It shouldn't be discarded. */
+ Assert(!UndoRecPtrIsDiscarded(xact_urp));
I think comments can be added to explain why it shouldn't
On Thu, Jul 18, 2019 at 4:41 PM Thomas Munro wrote:
>
> On Tue, Jul 16, 2019 at 8:39 AM Robert Haas wrote:
> > Thomas has already objected to another proposal to add functions that
> > turn 32-bit XIDs into 64-bit XIDs. Therefore, I feel confident in
> > predicting that he will likewise object
On Sat, Jul 20, 2019 at 12:40 PM Amit Kapila wrote:
>
> On Fri, Jul 19, 2019 at 6:35 PM Robert Haas wrote:
> >
> > On Fri, Jul 19, 2019 at 12:10 AM Amit Kapila
> > wrote:
> > > We are doing exactly what you have written in the last line of the
> > > next paragraph "stop the transaction from
On Fri, Jul 19, 2019 at 6:35 PM Robert Haas wrote:
>
> On Fri, Jul 19, 2019 at 12:10 AM Amit Kapila wrote:
> > We are doing exactly what you have written in the last line of the
> > next paragraph "stop the transaction from writing undo when the hash
> > table is already too full.". So we will
On Fri, Jul 19, 2019 at 6:35 PM Robert Haas wrote:
>
> On Fri, Jul 19, 2019 at 12:10 AM Amit Kapila wrote:
> > We are doing exactly what you have written in the last line of the
> > next paragraph "stop the transaction from writing undo when the hash
> > table is already too full.". So we will
On Fri, Jul 19, 2019 at 6:37 PM Robert Haas wrote:
>
> On Fri, Jul 19, 2019 at 7:54 AM Amit Khandekar wrote:
> > + * We just want to mask the cid in the undo record header. So
> > + * only if the partial record in the current page include the undo
> > + * record header then we need to mask the
On Fri, Jul 19, 2019 at 7:54 AM Amit Khandekar wrote:
> + * We just want to mask the cid in the undo record header. So
> + * only if the partial record in the current page include the undo
> + * record header then we need to mask the cid bytes in this page.
> + * Otherwise, directly jump to the
On Fri, Jul 19, 2019 at 12:10 AM Amit Kapila wrote:
> We are doing exactly what you have written in the last line of the
> next paragraph "stop the transaction from writing undo when the hash
> table is already too full.". So we will
> never face the problems related to repeated crash recovery.
On Thu, 9 May 2019 at 12:04, Dilip Kumar wrote:
> Patches can be applied on top of undo branch [1] commit:
> (cb777466d008e656f03771cf16ec7ef9d6f2778b)
>
> [1] https://github.com/EnterpriseDB/zheap/tree/undo
Below are some review points for 0009-undo-page-consistency-checker.patch :
+ /*
101 - 200 of 305 matches
Mail list logo
