logical/relation.c header description

Started by Amit Langoteover 5 years ago6 messages

amitlangote09@gmail.com

over 5 years ago

1 attachment(s)

Hi,

$subect needs fixing, because right now it says just:

/*-------------------------------------------------------------------------
* relation.c
* PostgreSQL logical replication

Attached find a patch to expand that a little bit.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Attachments:

logicalrep-file-header-desc.patchapplication/octet-stream; name=logicalrep-file-header-desc.patchDownload

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 9ee70a2..d08fe19 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -1,6 +1,6 @@
 /*-------------------------------------------------------------------------
  * relation.c
- *	   PostgreSQL logical replication
+ *	   PostgreSQL logical replication relation mapping routines
  *
  * Copyright (c) 2016-2020, PostgreSQL Global Development Group
  *

Amit Kapila

amit.kapila16@gmail.com

over 5 years ago

In reply to: Amit Langote (#1)

Re: logical/relation.c header description

On Thu, Sep 17, 2020 at 3:22 PM Amit Langote <amitlangote09@gmail.com> wrote:

 /*-------------------------------------------------------------------------
  * relation.c
- *    PostgreSQL logical replication
+ *    PostgreSQL logical replication relation mapping routines
  *
..
 * NOTES
 *   This file contains helper functions for logical replication relation
 *   mapping cache.

The new header title and NOTES section say the almost same thing. How
about changing the title as "PostgreSQL logical replication relation
mapping cache" and remove the NOTES section.

--
With Regards,
Amit Kapila.

Amit Langote

amitlangote09@gmail.com

over 5 years ago

In reply to: Amit Kapila (#2)

1 attachment(s)

Re: logical/relation.c header description

Thanks for taking a look.

On Thu, Sep 17, 2020 at 9:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 17, 2020 at 3:22 PM Amit Langote <amitlangote09@gmail.com> wrote:
/*-------------------------------------------------------------------------
* relation.c
- *    PostgreSQL logical replication
+ *    PostgreSQL logical replication relation mapping routines
*
..
* NOTES
*   This file contains helper functions for logical replication relation
*   mapping cache.
The new header title and NOTES section say the almost same thing. How
about changing the title as "PostgreSQL logical replication relation
mapping cache" and remove the NOTES section.

That makes sense.

Actually, I did consider expanding that NOTE to mention what the
module does but there are not actually that many interesting things in
there. If you would like to take a look at the text I had come up
with, please check the attached.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Attachments:

logicalrep-file-header-desc_v2.patchapplication/octet-stream; name=logicalrep-file-header-desc_v2.patchDownload

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 9ee70a2..4f31c95 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -1,6 +1,6 @@
 /*-------------------------------------------------------------------------
  * relation.c
- *	   PostgreSQL logical replication
+ *	   PostgreSQL logical replication relation mapping cache
  *
  * Copyright (c) 2016-2020, PostgreSQL Global Development Group
  *
@@ -8,8 +8,19 @@
  *	  src/backend/replication/logical/relation.c
  *
  * NOTES
- *	  This file contains helper functions for logical replication relation
- *	  mapping cache.
+ *	  Routines in this file mainly have to do with mapping the proprties of
+ *	  local replication  target relations to the properties of their remote
+ *	  counterpart.  This mapping is maintained in a LogicalRepRelMapEntry
+ *	  struct per target relation, cached in a local hash table keyed by the
+ *	  remote relation OID.
+ *
+ *	  Since replication actions targeting partitioned tables need to be
+ *	  applied to their leaf partitions instead, the mapping info must be
+ *	  maintained for each partition separately, even though all point to the
+ *	  same remote relation.  For that case, in addition to the target
+ *	  partitioned table's LogicalRepRelMapEntry, there is
+ *	  LogicalRepPartMapEntry for every partition touched so far, cached in
+ *	  another hash table which is keyed by partition OID.
  *
  *-------------------------------------------------------------------------
  */

Amit Kapila

amit.kapila16@gmail.com

over 5 years ago

In reply to: Amit Langote (#3)

1 attachment(s)

Re: logical/relation.c header description

On Thu, Sep 17, 2020 at 6:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for taking a look.

On Thu, Sep 17, 2020 at 9:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Sep 17, 2020 at 3:22 PM Amit Langote <amitlangote09@gmail.com> wrote:
/*-------------------------------------------------------------------------
* relation.c
- *    PostgreSQL logical replication
+ *    PostgreSQL logical replication relation mapping routines
*
..
* NOTES
*   This file contains helper functions for logical replication relation
*   mapping cache.
The new header title and NOTES section say the almost same thing. How
about changing the title as "PostgreSQL logical replication relation
mapping cache" and remove the NOTES section.
That makes sense.

Actually, I did consider expanding that NOTE to mention what the
module does but there are not actually that many interesting things in
there. If you would like to take a look at the text I had come up
with, please check the attached.

I am not very excited to add information about structures used in the
file especially when we have explained 'LogicalRepPartMapEntry' a few
lines after. I think we can leave explaining structures but otherwise,
it looks good. See attached.

--
With Regards,
Amit Kapila.

Attachments:

logicalrep-file-header-desc_v3.patchapplication/octet-stream; name=logicalrep-file-header-desc_v3.patchDownload

From 63242fce1fbf22320a996f960973f05ba7c0bb01 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 18 Sep 2020 08:55:12 +0530
Subject: [PATCH v2] Update file header for logical/relation.c.

---
 src/backend/replication/logical/relation.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index a60c73d74d..90650d159a 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -1,6 +1,6 @@
 /*-------------------------------------------------------------------------
  * relation.c
- *	   PostgreSQL logical replication
+ *	   PostgreSQL logical replication relation mapping cache
  *
  * Copyright (c) 2016-2020, PostgreSQL Global Development Group
  *
@@ -8,8 +8,9 @@
  *	  src/backend/replication/logical/relation.c
  *
  * NOTES
- *	  This file contains helper functions for logical replication relation
- *	  mapping cache.
+ *	  Routines in this file mainly have to do with mapping the properties
+ *	  of local replication target relations to the properties of their
+ *	  remote counterpart.
  *
  *-------------------------------------------------------------------------
  */
-- 
2.28.0.windows.1

Amit Langote

amitlangote09@gmail.com

over 5 years ago

In reply to: Amit Kapila (#4)

Re: logical/relation.c header description

On Fri, Sep 18, 2020 at 12:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 17, 2020 at 6:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
Thanks for taking a look.

On Thu, Sep 17, 2020 at 9:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Sep 17, 2020 at 3:22 PM Amit Langote <amitlangote09@gmail.com> wrote:
/*-------------------------------------------------------------------------
* relation.c
- *    PostgreSQL logical replication
+ *    PostgreSQL logical replication relation mapping routines
*
..
* NOTES
*   This file contains helper functions for logical replication relation
*   mapping cache.
The new header title and NOTES section say the almost same thing. How
about changing the title as "PostgreSQL logical replication relation
mapping cache" and remove the NOTES section.
That makes sense.

Actually, I did consider expanding that NOTE to mention what the
module does but there are not actually that many interesting things in
there. If you would like to take a look at the text I had come up
with, please check the attached.
I am not very excited to add information about structures used in the
file especially when we have explained 'LogicalRepPartMapEntry' a few
lines after. I think we can leave explaining structures but otherwise,
it looks good. See attached.

Works for me, thanks.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila

amit.kapila16@gmail.com

over 5 years ago

In reply to: Amit Langote (#5)

Re: logical/relation.c header description

On Fri, Sep 18, 2020 at 9:07 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Sep 18, 2020 at 12:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I am not very excited to add information about structures used in the
file especially when we have explained 'LogicalRepPartMapEntry' a few
lines after. I think we can leave explaining structures but otherwise,
it looks good. See attached.

Works for me, thanks.

Pushed, thanks!

--
With Regards,
Amit Kapila.