Opened 4 years ago

Closed 3 years ago

#4277 closed enhancement (fixed)

Implement PostgreSQL Host Data Source

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Kea1.1
Component: host-reservations Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket: 2
Total Hours: 81 Internal?: no

Description (last modified by tomek)

Once #4275 and #4276 are done, the next step is to implement PostgreSQL host data source. This task is major, therefore it's split into several tickets:

  • Implement host data source that handles Host structure with its fixed fields. This includes IPv4 reservations (this ticket)
  • Extend host data source to handle IPv6 reservations (#4278)
  • Extend host data source to handle client classes (#4279)
  • Extend host data source to handle options (#4280)


Subtickets

Attachments (1)

postgres_hr_classes.svg (35.9 KB) - added by tmark 3 years ago.
New dhcpsrv classes for HR in PostgreSQL

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by tomek

  • Description modified (diff)

comment:2 Changed 3 years ago by tmark

  • Owner set to tmark
  • Status changed from new to accepted

Changed 3 years ago by tmark

New dhcpsrv classes for HR in PostgreSQL

comment:3 Changed 3 years ago by tmark

The implementation includes Host Reservations for v4 and v6 with options and v6 reservations (tickets 4278 and 4280). The new class heirarchy shown in the following diagram mirrors the implementation done for MySQL. There was a bit of refactoring done in classes shared between HR and lease manager, primarily are relatively low levels:

  1. PgSqlExchange::getColumnnValue() variants were made static
  2. PsqlBindArray? was modified to create internally scoped strings for values that are converted to strings (such as numeric values)

New classes are shown in blue:

New dhcpsrv classes for HR in PostgreSQL

ChangeLog?:

11xx.   [func]      tmark
    The Postgresql backend now supports host reservations for both DHCPv4 and
    DHCPv6, with options and IPv6 reservations.
    (Trac #4277, git TBD)

comment:4 Changed 3 years ago by tmark

  • Add Hours to Ticket changed from 0 to 35
  • Estimated Difficulty 0 deleted
  • Owner changed from tmark to Unassigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 35

comment:5 Changed 3 years ago by tomek

  • Owner changed from Unassigned to tomek

comment:6 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 35 to 14
  • Owner changed from tomek to tmark
  • Total Hours changed from 35 to 49

First of all, thank you very much for taking up this task. Writing all
that functionality in one step is no small feat. Good work!

pgsql_connection.h
Line 278 - extra > at the end.

pgsql_exchange.cc
psqlBindArray::add(const uint8_t* data, const size_t len) - so what
would happen of someone passed NULL and 0?

The comment in line 73 suggests that addTempString could one day
replace add(string). If you think it's appropriate, please add @todo
to make it doxygen comment.

pgsql_exchange.h
The comment in line 34 says: "Other than vectors or buffers of binary
data, all other values are currently converted to their string
representation prior to sending them to PostgreSQL." Do you think
this changing this could be a potential performance improvement?

Line 56 and following have several OID types defined. Can you
order them by increasing values? Currently it goes ...20,23,21,25...

s/extroridinary/extraordinary

Side note: what a peculiar word. My understanding is that extra-
prefix has similar meaning to very. Nevertheless extraordinary means
not ordinary at all.

There are a number of new methods: rowCheck, colCheck, rowColCheck
etc. They're not used in any tests, but rather they're tested
indirectly. But that's not sufficient as this checks only positive
path, because our queries are consistent with the DB schema. I think
there should be a test that verifies they can indeed throw if DB
response is different than expected. This may be actually a good
test. I don't know what would happen if less than clueful sysadmin
messes up his DB schema.

s/garauntees/guarantees

isColumnNull and dumpRow should have @return parameters specified.

pgsql_lease_mgr.cc
You added a @todo in line 29. It should reference tickets #3557, #4530
and PR#9.

pgsql_host_data_source.h
Comment for ~PgSqlHostDataSource?: Yes, it's obvious it's destructor.
The comment should say what it does.

There's getAll(hwaddr, duid) call. I'm sad to see it, because I
believe we will not need it. It was useful when we only supported
two identifiers, but with the number of identifiers being 4, we could
define 6 such calls and the number is growing roughly exponential with
each new added identifier. Anyway, while adding this implementation
at this time is necessary, I hope we will do an update and remove
this and other unused call types.

Some of the get(...) calls implemented are no longer used. They are
still in the base class, and therefore implemented in all backends,
but they're not used. The current logic uses get(identifier-type,
identifier, subnet-id) calls as the future-proof (or so we think)
calling that is able to cope with any identifiers we could possibly
imagine. I have created ticket #4536. If/when it is implemented, it
will remove parts of the code added in this ticket. If you're ok with
this, let's carry on. Alternatively, we could put #4277 on hold, wait
till #4536 is merged, then rebase #4277 and trim down the code.

I'm not sure, but it seems likely that the following calls are
to be removed:
getAll(hwaddr, duid)
get4(subnet-id, hwaddr, duid)
get6(subnet-id, duid, hwaddr)
get6(prefix, prefix-len)

Comment in line 148 says "Implementations of this method should guard
against invalid addresses, such as IPv6 address.". This is something
that sounded ok in the the base class header, but here it should
be either removed or better yet, turned into a definite statemnt
"this method guards ...". Similar comment applies to text starting
in line 160.

Comment for add() method (line 201 and following) should enumerate all
checks that are conducted to ensure that there is no duplicate. If
none are implemented, the text should be removed. The comment should
explain what happens if a duplicate is found (an exception is thrown?)

getType() comment should not mention mysql or memfile.

So what exactly does getName() return? I think it should return name
of the database (e.g. keatest), not the type of backend. That's what
getType() is for. In any case, the description should be updated:
it does not return "mysql". At least I hope it doesn't.

Do you think it would be useful to explicitly enumerate all identifier
types currently supported? It seems that we currently support 4
identifiers: hw-address, duid, circuit-id and client-id. Unless
someone knows exactly what identifier-type is, he will be completely
clueless if he just reads the header file. I don't really know what's
the best way to have such a list would be. Maybe list them and then
have a pointer to host_identifier_type table in dhcpdb_create.pgsql?

pgsql_host_data_source.cc

Why do you need two separate pointers: host_ipv6_exchange_ and
host_ipv46_exchange_? Couldn't they be combined into a single
instance?

OPTION_VALUE_MAX_LEN is interesting. I hope there is nobody willing to
define longer options. Anyway, this definitely should be documented.
I presume there's a separate ticket for it. I can see this being
possibly tweaked by someone needing insanely large options. The only
use case for such a thing I can imagine would be sedhcpv6 with their
idea to store certificates in DHCPv6 options. I'm not sure how large
these are, but when asked about their size I was told "LARGE".

s/inovke/invoke/

s/It makes it determination/It makes its determination/

retrieveOption() in line 391. It's unclear to me why the code needs
to make any determinations regarding whether an option is a new
instance or not. If there are 2 hosts and each of the has 3 options,
the left join queries will return 6 lines, but each line will have
unique option and the options will never repeat themselves.

The line 426 asks whether max length of 8K should be enforced. The
schema doesn't impose such limits, so I suppose we could leave it off.
Maybe add a cautionary text in the docs that you are doing something
less than sane if your options definitions are longer than 8K.

Looking at the code in line 448, I think you should take a quick look
at PR#24 on github. They propose to tweak the option spaces slightly.
There's nothing to do with it yet, as your code will likely go in
first, but I thought I should mention that in case we decide to
postpone #4277 until #4536 is done.

A question from non-native speaker about line 523: you wrote indexes.
As I understand it both indexes and indices are valid forms. Is
one of them more popular than other?

The comment around 620 explains that data can contain duplicated host
or options entries. How is that possible? I get the duplicate host
case, but not the duplicate option case. If we have 2 hosts, each
with 3 options, we will get 6 rows. Hosts will be duplicated, but
the options won't.

s/sitations/situations/

Line 924: the comment mentions IA_TA, but that type is not supported.

Constants in lines 954 through 973. I'm slightly concerned that the
proper operation on this code relies on two independent lists being
consistent. As far as I can tell every host data source has this
characterstic, so there's probably not much to be done here.

Comments in lines 1092 and 1093 are not aligned with other comments.

Line 1125 s/@returns/@return/

Line 1249 and following. The comment after 8 is confusing. It
specifies the query id, but the 8 represents the number of columns.

pgsql_host_data_source_unittest.cc
There is no checkTimeConversion unit-test. I admit it has little to
do with host data source, but it's a useful test, especially in the
context of people asking for "infinite" leases. If you think it's
ok to implement such a ticket in this ticket, it's great. If not,
please at least add a @todo in the code and open a ticket for it.

There's little sense in adding hwaddrOrClientId1 or hwaddrOrClientId2
boilerplates as the calls they were supposed to test will go away,
hopefully soon. In my opinion it's very much ok to remove them.

multipleClientClasses4, multipleClientClasses6 and
multipleClientClassesBoth should be implemented. They are supported by
PgSQL, so there's no reason why they could remain unimplemented.


The code compiles and unit-tests pass on Ubuntu 15.10 x64.


The proposed changelog is ok, but please correct the capitalization
(s/Postgresql/PostgreSQL/).


I don't see any dedicated ticket for doing Developer's Guide update.
Do you think you could put one paragraph in the developer's guide and
include the class diagram you generated?


You mentioned this work addresses #4278 and #4280. I see that the code
also does client classes, so it also covers #4279, right?

comment:7 Changed 3 years ago by tomek

This code also addresses #4323.

comment:8 follow-up: Changed 3 years ago by tmark

  • Add Hours to Ticket changed from 14 to 24
  • Owner changed from tmark to tomek
  • Total Hours changed from 49 to 73

Replying to tomek:

First of all, thank you very much for taking up this task. Writing all
that functionality in one step is no small feat. Good work!

You're welcome, although the bulk of the design and structure came from the MySQL HR work done by Adam and Marcin. I basically ported it to PostgreSQL.

pgsql_connection.h
Line 278 - extra > at the end.

Got it.

pgsql_exchange.cc
psqlBindArray::add(const uint8_t* data, const size_t len) - so what
would happen of someone passed NULL and 0?

Well it would explode. Two choices here, one would be to throw an exception, the other is test for NULL and instead call psqlBindArray::addNull(BINARY_FMT). Initially I went with the latter but then felt it was best if the caller needs to knowingly set a value to NULL, so this method allong with PsqlBindArray::add(const char*) now both throw if the pointer is NULL.

A size of zero is fine, you end up with an empty entry.

The comment in line 73 suggests that addTempString could one day
replace add(string). If you think it's appropriate, please add @todo
to make it doxygen comment.

Done and have expanded the thought.

pgsql_exchange.h
The comment in line 34 says: "Other than vectors or buffers of binary
data, all other values are currently converted to their string
representation prior to sending them to PostgreSQL." Do you think
this changing this could be a potential performance improvement?

Months ago, when I first retooled the PosgreSQL contribution I explored this by doing some testing with code that used binary values vs string values. As this was my instinctual reaction when I first saw the code. Remarkably, the string valued code was marginally faster. The basic issue with using binary values is that PostgreSQL defines 3 integer types:

BIGINT - signed 8 byte
INT - signed 4 byte
SMALLINT - signed 2 byte

and the values must be in network byte order. This means that rather than binding directly to a object member such as Lease::subnet_id_, one must convert the value to one of the above forms in network byte order first. And of course, one must reverse the process on received data.

It might be worth another pass at this when we undertake PostgreSQL performance as my testing may have been flawed or inadequate or my test implemenation might not have been handling values as efficiently as one could.

Also with the newly added low level unit tests (described later) it would be a good deal easier to explore this.


Line 56 and following have several OID types defined. Can you
order them by increasing values? Currently it goes ...20,23,21,25...

Sure.

s/extroridinary/extraordinary

Side note: what a peculiar word. My understanding is that extra-
prefix has similar meaning to very. Nevertheless extraordinary means
not ordinary at all.

Peculiar and apparently hard for me to spell.
"extra" is really more like "in addition to", "beyond", or "exceeds"; such as
"extraterrestrial" or "extrasensory".

There are a number of new methods: rowCheck, colCheck, rowColCheck
etc. They're not used in any tests, but rather they're tested
indirectly. But that's not sufficient as this checks only positive
path, because our queries are consistent with the DB schema. I think
there should be a test that verifies they can indeed throw if DB
response is different than expected. This may be actually a good
test. I don't know what would happen if less than clueful sysadmin
messes up his DB schema.

Being able to test the basics of writing and reading data have been lacking from the inception and as been something that I've wanted to correct for a long time. I have added a test fixture and tests to pgsql_exchange_unittest.cc which uses its own table to exercise all of the data types.

We may be able to abstract a generic test structure so that all backends have this level of testing, but admittedly at this level things are very specific to the backend implementations. We can always create a ticket to examine this. Certainly all of the
backends should verify that all of its data types work.

s/garauntees/guarantees

Another word I routinely misspell.

isColumnNull and dumpRow should have @return parameters specified.

pgsql_lease_mgr.cc
You added a @todo in line 29. It should reference tickets #3557, #4530
and PR#9.

Done.

pgsql_host_data_source.h
Comment for ~PgSqlHostDataSource?: Yes, it's obvious it's destructor.
The comment should say what it does.

Actually it does nothing directly but I added a bit for you.

There's getAll(hwaddr, duid) call. I'm sad to see it, because I
believe we will not need it. It was useful when we only supported
two identifiers, but with the number of identifiers being 4, we could
define 6 such calls and the number is growing roughly exponential with
each new added identifier. Anyway, while adding this implementation
at this time is necessary, I hope we will do an update and remove
this and other unused call types.

Some of the get(...) calls implemented are no longer used. They are
still in the base class, and therefore implemented in all backends,
but they're not used. The current logic uses get(identifier-type,
identifier, subnet-id) calls as the future-proof (or so we think)
calling that is able to cope with any identifiers we could possibly
imagine. I have created ticket #4536. If/when it is implemented, it
will remove parts of the code added in this ticket. If you're ok with
this, let's carry on. Alternatively, we could put #4277 on hold, wait
till #4536 is merged, then rebase #4277 and trim down the code.

I'm not sure, but it seems likely that the following calls are
to be removed:
getAll(hwaddr, duid)
get4(subnet-id, hwaddr, duid)
get6(subnet-id, duid, hwaddr)
get6(prefix, prefix-len)

This will be addressed under ticket #4536.

Comment in line 148 says "Implementations of this method should guard
against invalid addresses, such as IPv6 address.". This is something
that sounded ok in the the base class header, but here it should
be either removed or better yet, turned into a definite statemnt
"this method guards ...". Similar comment applies to text starting
in line 160.

I have added a check for a V6 address. The MySQL version of this method is also missing this test.

Comment for add() method (line 201 and following) should enumerate all
checks that are conducted to ensure that there is no duplicate. If
none are implemented, the text should be removed. The comment should
explain what happens if a duplicate is found (an exception is thrown?)

Added commentary.

getType() comment should not mention mysql or memfile.

Got it.

So what exactly does getName() return? I think it should return name
of the database (e.g. keatest), not the type of backend. That's what
getType() is for. In any case, the description should be updated:
it does not return "mysql". At least I hope it doesn't.

Ok, so I didn't fix all the commentary when I ported this stuff. ;)
It does in fact, return the connection parameter "name" which amounts to
the name of the database, such as "keatest".

Commentary is now accurate.

Do you think it would be useful to explicitly enumerate all identifier
types currently supported? It seems that we currently support 4
identifiers: hw-address, duid, circuit-id and client-id. Unless
someone knows exactly what identifier-type is, he will be completely
clueless if he just reads the header file. I don't really know what's
the best way to have such a list would be. Maybe list them and then
have a pointer to host_identifier_type table in dhcpdb_create.pgsql?

Added the list to the PgSqlHostDataSource? commentary.

pgsql_host_data_source.cc

Why do you need two separate pointers: host_ipv6_exchange_ and
host_ipv46_exchange_? Couldn't they be combined into a single
instance?

A somewhat subtle difference here. The former is instantiated with mode of DHCP6_ONLY, the latter is DHCP4_AND_DHCP6. This affects which options are expected in the result sets, so no they aren't interchangeable.

I believe the intent was performance driven here. Create exchanges once, that are geared to handle specific result sets and use them for specific cases. I am somewhat on the fence about it but can see the reasoning behind it.

OPTION_VALUE_MAX_LEN is interesting. I hope there is nobody willing to
define longer options. Anyway, this definitely should be documented.
I presume there's a separate ticket for it. I can see this being
possibly tweaked by someone needing insanely large options. The only
use case for such a thing I can imagine would be sedhcpv6 with their
idea to store certificates in DHCPv6 options. I'm not sure how large
these are, but when asked about their size I was told "LARGE".

I just blindly followed the others here. Document it where?

s/inovke/invoke/

Got it.

s/It makes it determination/It makes its determination/

Got it.

retrieveOption() in line 391. It's unclear to me why the code needs
to make any determinations regarding whether an option is a new
instance or not. If there are 2 hosts and each of the has 3 options,
the left join queries will return 6 lines, but each line will have
unique option and the options will never repeat themselves.

I scratched my head about this too. At first. If there are only v4 options or v6 options, then yes you are correct. However it gets more interesting when there are two left joins, such as both v4 options and v6 options. Then you get result sets which look like this:

host 1 v4 opt1 v6 opt1
host 1 v4 opt1 v6 opt2
host 1 v4 opt1 v6 opt3
host 1 v4 opt2 v6 opt1
host 1 v4 opt2 v6 opt2
host 1 v4 opt2 v6 opt3
host 2 v4 opt1 v6 opt1
host 2 v4 opt2 v6 opt1

You get a lot of repeated information. You run into the same sort of thing when you add in v6 reservations. Once you add in a second left join you start getting repeated data.
I have added this to the commentary.

The line 426 asks whether max length of 8K should be enforced. The
schema doesn't impose such limits, so I suppose we could leave it off.
Maybe add a cautionary text in the docs that you are doing something
less than sane if your options definitions are longer than 8K.

MySqlHostDataSource? defines a maximum of 8K but this has more to do with how it binds data than anything else. Its schema does not impose restrictions either. I have taken out the comment.

Looking at the code in line 448, I think you should take a quick look
at PR#24 on github. They propose to tweak the option spaces slightly.
There's nothing to do with it yet, as your code will likely go in
first, but I thought I should mention that in case we decide to
postpone #4277 until #4536 is done.

Will defer this.

A question from non-native speaker about line 523: you wrote indexes.
As I understand it both indexes and indices are valid forms. Is
one of them more popular than other?

I think in the US you are more likely to hear "indexes". I've always said "indexes" but then I'm barely literate. On the other side of the pond it's probably "indices".

The comment around 620 explains that data can contain duplicated host
or options entries. How is that possible? I get the duplicate host
case, but not the duplicate option case. If we have 2 hosts, each
with 3 options, we will get 6 rows. Hosts will be duplicated, but
the options won't.

Refer to the prior discussion on mulitple left joins.
I've expanded the commentary to explain it more clearly.

s/sitations/situations/

Oops.

Line 924: the comment mentions IA_TA, but that type is not supported.

Got it.

Constants in lines 954 through 973. I'm slightly concerned that the
proper operation on this code relies on two independent lists being
consistent. As far as I can tell every host data source has this
characterstic, so there's probably not much to be done here.

I'm not thrilled about this either but given the time constraint and a desire to maintain a degree of symmetry between the backend implementations I left it alone.

Comments in lines 1092 and 1093 are not aligned with other comments.

Picky, picky.

Line 1125 s/@returns/@return/

Check.

Line 1249 and following. The comment after 8 is confusing. It
specifies the query id, but the 8 represents the number of columns.

I have moved the comment with id to be first above each statement. Cleaner this way.

pgsql_host_data_source_unittest.cc
There is no checkTimeConversion unit-test. I admit it has little to
do with host data source, but it's a useful test, especially in the
context of people asking for "infinite" leases. If you think it's
ok to implement such a ticket in this ticket, it's great. If not,
please at least add a @todo in the code and open a ticket for it.

Actually there wae a test for this in pgsql_exchange_unittest.cc - TEST(PgSqlExchangeTest?, convertTimeTest).

This has been replaced with PgSqlBasicsTest? stuff.

There's little sense in adding hwaddrOrClientId1 or hwaddrOrClientId2
boilerplates as the calls they were supposed to test will go away,
hopefully soon. In my opinion it's very much ok to remove them.

multipleClientClasses4, multipleClientClasses6 and
multipleClientClassesBoth should be implemented. They are supported by
PgSQL, so there's no reason why they could remain unimplemented.

Testing for host client classes appears incomplete in GenericHostDataSourceTest? as this hasn't been done yet for MySQL. I would prefer to address this as part of #4279, simply because #4277 is already quite large.


The code compiles and unit-tests pass on Ubuntu 15.10 x64.


The proposed changelog is ok, but please correct the capitalization
(s/Postgresql/PostgreSQL/).


I don't see any dedicated ticket for doing Developer's Guide update.
Do you think you could put one paragraph in the developer's guide and
include the class diagram you generated?

I added a section under the "libdhcpsrv - Server DHCP library" section


You mentioned this work addresses #4278 and #4280. I see that the code
also does client classes, so it also covers #4279, right?

When I started this ticket, Marcin suggested I do #4278 and #4280 as well since MySQL
impl had already done them, but not to do #4279 as this had not yet been done for MySQL.
We have unofficially let MySQL lead the implemention. While I was doing this ticket
I went ahead and added the client class column support, primarily because I there.
I would still prefer to address the testing under #4279.

Phew....

I verified that distcheck works.

Ready for re-review.

comment:9 in reply to: ↑ 8 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 24 to 6
  • Owner changed from tomek to tmark
  • Total Hours changed from 73 to 79

I left only those items that require further comments.

Replying to tmark:

Replying to tomek:

First of all, thank you very much for taking up this task. Writing all
that functionality in one step is no small feat. Good work!

You're welcome, although the bulk of the design and structure came from the MySQL HR work done by Adam and Marcin. I basically ported it to PostgreSQL.

It took a lot of time of everyone involved, but the result seems to be much better than what we started with.

Being able to test the basics of writing and reading data have been lacking from the inception and as been something that I've wanted to correct for a long time. I have added a test fixture and tests to pgsql_exchange_unittest.cc which uses its own table to exercise all of the data types.


We may be able to abstract a generic test structure so that all backends have this level of testing, but admittedly at this level things are very specific to the backend implementations. We can always create a ticket to examine this. Certainly all of the
backends should verify that all of its data types work.

I don't think everything has to be done in generic way, especially with Cassandra being next in the pipeline. What you did is great. We should one day develop similar code for MySQL and Cassandra. Yes, there will be some duplication of the code, but it's much better than develop yet another abstraction layer that would make the test more difficult to use.

OPTION_VALUE_MAX_LEN is interesting. I hope there is nobody willing to
define longer options. Anyway, this definitely should be documented.
I presume there's a separate ticket for it. I can see this being
possibly tweaked by someone needing insanely large options. The only
use case for such a thing I can imagine would be sedhcpv6 with their
idea to store certificates in DHCPv6 options. I'm not sure how large
these are, but when asked about their size I was told "LARGE".

I just blindly followed the others here. Document it where?

In sections 7.3.5, 7.3.6, 8.3.5 and 8.3.6. Adding a sentence like this would address this issue:
"Currently maximum length of options specified per host is arbitrarily set to 4096 bytes."
One problem is that the 4277 code was branched before those sections made it to master. I don't want this ticket to be delayed any further, so I added a comment 5 in #3684 that explains what has to be done. So nothing to do in #4277 regarding this one.

retrieveOption() in line 391. It's unclear to me why the code needs
to make any determinations regarding whether an option is a new
instance or not. If there are 2 hosts and each of the has 3 options,
the left join queries will return 6 lines, but each line will have
unique option and the options will never repeat themselves.

I scratched my head about this too. At first. If there are only v4 options or v6 options, then yes you are correct. However it gets more interesting when there are two left joins, such as both v4 options and v6 options. Then you get result sets which look like this:
host 1 v4 opt1 v6 opt1
host 1 v4 opt1 v6 opt2
host 1 v4 opt1 v6 opt3
host 1 v4 opt2 v6 opt1
host 1 v4 opt2 v6 opt2
host 1 v4 opt2 v6 opt3
host 2 v4 opt1 v6 opt1
host 2 v4 opt2 v6 opt1

You get a lot of repeated information. You run into the same sort of thing when you add in v6 reservations. Once you add in a second left join you start getting repeated data.
I have added this to the commentary.

Thanks. I have a gut feeling that doing multiple joins and then compacting the result is not the most efficient way of handling those things. On the other hand, this issues one query rather than four and in most cases there are no options or v6 reservations, so it is optimized for a case of usually empty set.

This has been replaced with PgSqlBasicsTest? stuff.

Testing for host client classes appears incomplete in GenericHostDataSourceTest? as this hasn't been done yet for MySQL. I would prefer to address this as part of #4279, simply because #4277 is already quite large.

Agree.

I have added a check for a V6 address. The MySQL version of this method is also missing this test.

Added @todo for them (there's also get6() method that would gladly accept v4 address).


Just one minor comment:

pgsql_host_data_source.h: The comment for add() method mentions rules
which, when violated, will cause DuplicateEntry? exception. There's a
rule for v4 address being duplicated, but I can't see similar
exception for v6 address. If there is one and it's simply not
documented, please update the comment. If there isn't, please file a
new ticket for it. It will likely end up in outstanding, but at least
we will be aware of the shortcomings.

I did review your new basics tests. They look good. Couple lines were too wrapped, so I reorganized it slightly and pushed my changes. Please pull. If you agree with the changes, then the only outstanding thing is the duplicate v6 address thing in add().

I just upgraded to Ubuntu 16.04 x64. The code compiles fine and unit-tests pass. (I encountered a problem with unit-tests in libdhcp_ddns, but these are completely unrelated).

I don't need to see this ticket again.

comment:10 Changed 3 years ago by tmark

  • Add Hours to Ticket changed from 6 to 2
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 79 to 81

There is a constraint on the ipv6_reservations table that requires the addressa and prefix_len combination to be unique and the commentary accordingly. I verfied that MySQL also has this constraint.

Changes merged with git# ac1eaa1026987c2d86d57b4aa0dc9a4d093787f0
Added ChangeLog? entry 1145.

Ticket is complete.

Note: See TracTickets for help on using tickets.